Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1717035 > unrolled thread

[PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

Back to article view | Back to linux.kernel

Contents

  [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only) Long Li <longli@exchange.microsoft.com> - 2017-08-22 07:40 +0200
    Re: [PATCH] storvsc: do not assume SG list is continuous when doing  bounce buffers (for 4.1 stable only) Christoph Hellwig <hch@infradead.org> - 2017-08-22 08:30 +0200
      RE: [PATCH] storvsc: do not assume SG list is continuous when doing  bounce buffers (for 4.1 stable only) Long Li <longli@microsoft.com> - 2017-08-22 21:20 +0200
        Re: [PATCH] storvsc: do not assume SG list is continuous when doing  bounce buffers (for 4.1 stable only) Christoph Hellwig <hch@infradead.org> - 2017-08-23 08:50 +0200
          Re: [PATCH] storvsc: do not assume SG list is continuous when doing  bounce buffers (for 4.1 stable only) Greg KH <gregkh@linuxfoundation.org> - 2017-08-23 15:50 +0200
          Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only) "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-08-23 17:30 +0200

#1717035 — [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

From: Long Li <longli@microsoft.com>

This patch is for linux-stable 4.1 branch only.

storvsc checks the SG list for gaps before passing them to Hyper-v device.
If there are gaps, data is copied to a bounce buffer and a continuous data
buffer is passed to Hyper-V.

The check on gaps assumes SG list is continuous, and not chained. This is
 not always true. Failing the check may result in incorrect I/O data
passed to the Hyper-v device.

This code path is not used post Linux 4.1.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6c52d14..14dc5c6 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -584,17 +584,18 @@ static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count)
 	for (i = 0; i < sg_count; i++) {
 		if (i == 0) {
 			/* make sure 1st one does not have hole */
-			if (sgl[i].offset + sgl[i].length != PAGE_SIZE)
+			if (sgl->offset + sgl->length != PAGE_SIZE)
 				return i;
 		} else if (i == sg_count - 1) {
 			/* make sure last one does not have hole */
-			if (sgl[i].offset != 0)
+			if (sgl->offset != 0)
 				return i;
 		} else {
 			/* make sure no hole in the middle */
-			if (sgl[i].length != PAGE_SIZE || sgl[i].offset != 0)
+			if (sgl->length != PAGE_SIZE || sgl->offset != 0)
 				return i;
 		}
+		sgl = sg_next(sgl);
 	}
 	return -1;
 }
-- 
2.7.4

[toc] | [next] | [standalone]

#1717057 — Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

Wouldn't it make sense to backport the changes to set the virt_boundary
(which probably still is the SG_GAPS flag in such an old kernel)?

[toc] | [prev] | [next] | [standalone]

#1717680 — RE: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

 
> Wouldn't it make sense to backport the changes to set the virt_boundary
> (which probably still is the SG_GAPS flag in such an old kernel)?

We can make storvsc use SG_GAPS. But the following patch is missing in 4.1 stable block layer to make this work on some I/O situations. Backporting is more difficult and affect other code.

commit 5e7c4274a70aa2d6f485996d0ca1dad52d0039ca
Author: Jens Axboe <axboe@fb.com>
Date:   Thu Sep 3 19:28:20 2015 +0300

    block: Check for gaps on front and back merges
    
    We are checking for gaps to previous bio_vec, which can
    only detect back merges gaps. Moreover, at the point where
    we check for a gap, we don't know if we will attempt a back
    or a front merge. Thus, check for gap to prev in a back merge
    attempt and check for a gap to next in a front merge attempt.
    
    Signed-off-by: Jens Axboe <axboe@fb.com>
    [sagig: Minor rename change]
    Signed-off-by: Sagi Grimberg <sagig@mellanox.com>

[toc] | [prev] | [next] | [standalone]

#1718082 — Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

Ok.  If the stable maintainers are ok with your small fix
I'm not going to complain too loudly.  But I'm always worried about
stable trees divering too much from mainline.

[toc] | [prev] | [next] | [standalone]

#1718359 — Re: [PATCH] storvsc: do not assume SG list is continuous when doing bounce buffers (for 4.1 stable only)

On Tue, Aug 22, 2017 at 11:43:19PM -0700, Christoph Hellwig wrote:
> Ok.  If the stable maintainers are ok with your small fix
> I'm not going to complain too loudly.  But I'm always worried about
> stable trees divering too much from mainline.

Given that 90% of the time we do this, something breaks, you have a
right to be worried...

[toc] | [prev] | [next] | [standalone]

#1718449

Christoph,

> Ok.  If the stable maintainers are ok with your small fix
> I'm not going to complain too loudly.  But I'm always worried about
> stable trees divering too much from mainline.

The seemingly innocuous transition from SG_GAPS to virt boundary has
caused several data corruption regressions in the distro kernels. So has
the corresponding conversion of storvsc.

As a result, getting the current upstream code into 4.1 would mean
backporting and testing a significant amount of both block layer and
driver code. I don't think it's worth the risk. This patch is simple and
the path of least resistance.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web