[PATCH --resend 0/2] fix writesame

[PATCH --resend 0/2] fix writesame

[Shaohua Li]

This is the writesame issue fix reported in bugzilla 118581. Sitsofe doesn't
reply to me if he tested these, but they do work for me with a scsi_debug test.
I'm going to apply them for 4.11 if no objection.

Shaohua Li (2):
  md: disable WRITE SAME if it fails for linear/raid0
  md/multipath: disable WRITE SAME if it fails for multipath

 drivers/md/linear.c    |  6 +++++-
 drivers/md/md.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/md.h        |  2 ++
 drivers/md/multipath.c |  4 ++++
 drivers/md/raid0.c     |  6 +++++-
 5 files changed, 61 insertions(+), 2 deletions(-)

-- 
2.9.3

[PATCH --resend 1/2] md: disable WRITE SAME if it fails for linear/raid0

[Shaohua Li]

This makes md do the same thing as dm for write same IO failure. Please
see 7eee4ae(dm: disable WRITE SAME if it fails) for details why we need
this.

Also reported here: https://bugzilla.kernel.org/show_bug.cgi?id=118581

Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/linear.c |  6 +++++-
 drivers/md/md.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/md.h     |  2 ++
 drivers/md/raid0.c  |  6 +++++-
 4 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/md/linear.c b/drivers/md/linear.c
index 26a73b2..bebc834 100644
--- a/drivers/md/linear.c
+++ b/drivers/md/linear.c
@@ -291,7 +291,11 @@ static void linear_make_request(struct mddev *mddev, struct bio *bio)
 				trace_block_bio_remap(bdev_get_queue(split->bi_bdev),
 						      split, disk_devt(mddev->gendisk),
 						      bio_sector);
-			generic_make_request(split);
+			if (bio_op(split) == REQ_OP_WRITE_SAME)
+				generic_make_request(md_writesame_setup(mddev,
+								split));
+			else
+				generic_make_request(split);
 		}
 	} while (split != bio);
 	return;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 13020e5..7354f0b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -312,6 +312,51 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
 	return BLK_QC_T_NONE;
 }
 
+struct md_writesame_data {
+	struct bio *orig_bio;
+	struct mddev *mddev;
+	struct bio cloned_bio;
+};
+
+static void md_writesame_endio(struct bio *bio)
+{
+	struct md_writesame_data *data = bio->bi_private;
+
+	if (bio->bi_error == -EREMOTEIO &&
+	    !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
+		data->mddev->queue->limits.max_write_same_sectors = 0;
+
+	data->orig_bio->bi_error = bio->bi_error;
+	bio_endio(data->orig_bio);
+
+	kfree(data);
+}
+
+struct bio *md_writesame_setup(struct mddev *mddev, struct bio *bio)
+{
+	struct md_writesame_data *data;
+	struct bio *cloned_bio;
+
+	/*
+	 * this failure means we ignore a chance to handle writesame failure,
+	 * which isn't critcal, we can handle the failure if new writesame IO
+	 * comes
+	 */
+	data = kmalloc(sizeof(*data), GFP_NOIO | __GFP_NORETRY);
+	if (!data)
+		return bio;
+	cloned_bio = &data->cloned_bio;
+	data->mddev = mddev;
+	data->orig_bio = bio;
+	bio_init(cloned_bio, NULL, 0);
+	__bio_clone_fast(cloned_bio, bio);
+
+	cloned_bio->bi_private = data;
+	cloned_bio->bi_end_io = md_writesame_endio;
+	return cloned_bio;
+}
+EXPORT_SYMBOL_GPL(md_writesame_setup);
+
 /* mddev_suspend makes sure no new requests are submitted
  * to the device, and that any requests that have been submitted
  * are completely handled.
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 2a51403..5c983e9 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -710,4 +710,6 @@ static inline void mddev_clear_unsupported_flags(struct mddev *mddev,
 {
 	mddev->flags &= ~unsupported_flags;
 }
+
+extern struct bio *md_writesame_setup(struct mddev *mddev, struct bio *bio);
 #endif /* _MD_MD_H */
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 848365d..e38636e 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -503,7 +503,11 @@ static void raid0_make_request(struct mddev *mddev, struct bio *bio)
 				trace_block_bio_remap(bdev_get_queue(split->bi_bdev),
 						      split, disk_devt(mddev->gendisk),
 						      bio_sector);
-			generic_make_request(split);
+			if (bio_op(split) == REQ_OP_WRITE_SAME)
+				generic_make_request(md_writesame_setup(mddev,
+							split));
+			else
+				generic_make_request(split);
 		}
 	} while (split != bio);
 }
-- 
2.9.3

[PATCH --resend 2/2] md/multipath: disable WRITE SAME if it fails for multipath

[Shaohua Li]

This is the part for multipath. Since multipatch already attaches
private data into original bio, we just disable write same there.

Signed-off-by: Shaohua Li <shli@fb.com>
---
 drivers/md/multipath.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index aa8c4e5c..5ee9579 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -89,6 +89,10 @@ static void multipath_end_request(struct bio *bio)
 	struct mpconf *conf = mp_bh->mddev->private;
 	struct md_rdev *rdev = conf->multipaths[mp_bh->path].rdev;
 
+	if (bio_op(bio) == REQ_OP_WRITE_SAME && bio->bi_error == -EREMOTEIO &&
+	    !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
+		mp_bh->mddev->queue->limits.max_write_same_sectors = 0;
+
 	if (!bio->bi_error)
 		multipath_end_bh_io(mp_bh, 0);
 	else if (!(bio->bi_opf & REQ_RAHEAD)) {
-- 
2.9.3

Re: [PATCH --resend 1/2] md: disable WRITE SAME if it fails for linear/raid0

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

On Mon, Feb 13 2017, Shaohua Li wrote:

> This makes md do the same thing as dm for write same IO failure. Please
> see 7eee4ae(dm: disable WRITE SAME if it fails) for details why we need
> this.
>
> Also reported here: https://bugzilla.kernel.org/show_bug.cgi?id=118581
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/linear.c |  6 +++++-
>  drivers/md/md.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/md/md.h     |  2 ++
>  drivers/md/raid0.c  |  6 +++++-
>  4 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/linear.c b/drivers/md/linear.c
> index 26a73b2..bebc834 100644
> --- a/drivers/md/linear.c
> +++ b/drivers/md/linear.c
> @@ -291,7 +291,11 @@ static void linear_make_request(struct mddev *mddev, struct bio *bio)
>  				trace_block_bio_remap(bdev_get_queue(split->bi_bdev),
>  						      split, disk_devt(mddev->gendisk),
>  						      bio_sector);
> -			generic_make_request(split);
> +			if (bio_op(split) == REQ_OP_WRITE_SAME)
> +				generic_make_request(md_writesame_setup(mddev,
> +								split));
> +			else
> +				generic_make_request(split);
>  		}
>  	} while (split != bio);
>  	return;
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 13020e5..7354f0b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -312,6 +312,51 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
>  	return BLK_QC_T_NONE;
>  }
>  
> +struct md_writesame_data {
> +	struct bio *orig_bio;
> +	struct mddev *mddev;
> +	struct bio cloned_bio;
> +};
> +
> +static void md_writesame_endio(struct bio *bio)
> +{
> +	struct md_writesame_data *data = bio->bi_private;
> +
> +	if (bio->bi_error == -EREMOTEIO &&
> +	    !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
> +		data->mddev->queue->limits.max_write_same_sectors = 0;

What would be *really* nice is if a block device could send a
reconfigure message to its 'holder' (bd_holder).  This could include
device size changes and, for this, changes to max_write_same_sectors.
There are probably other changes that can usefully be propagated.

But for this patch, wouldn't it be easier, and maybe more efficient, to
do

  if (bio_op(split) == REQ_OP_WRITE_SAME &&
      !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
           mddev->queue->limits.max_write_same_sectors = 0;
  generic_make_request(split);

???
If there is some reason that can't work, then the patch as it stands
look OK to me.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH --resend 1/2] md: disable WRITE SAME if it fails for linear/raid0

[Shaohua Li]

On Tue, Feb 14, 2017 at 10:42:32AM +1100, Neil Brown wrote:
> On Mon, Feb 13 2017, Shaohua Li wrote:
> 
> > This makes md do the same thing as dm for write same IO failure. Please
> > see 7eee4ae(dm: disable WRITE SAME if it fails) for details why we need
> > this.
> >
> > Also reported here: https://bugzilla.kernel.org/show_bug.cgi?id=118581
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> >  drivers/md/linear.c |  6 +++++-
> >  drivers/md/md.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/md/md.h     |  2 ++
> >  drivers/md/raid0.c  |  6 +++++-
> >  4 files changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/linear.c b/drivers/md/linear.c
> > index 26a73b2..bebc834 100644
> > --- a/drivers/md/linear.c
> > +++ b/drivers/md/linear.c
> > @@ -291,7 +291,11 @@ static void linear_make_request(struct mddev *mddev, struct bio *bio)
> >  				trace_block_bio_remap(bdev_get_queue(split->bi_bdev),
> >  						      split, disk_devt(mddev->gendisk),
> >  						      bio_sector);
> > -			generic_make_request(split);
> > +			if (bio_op(split) == REQ_OP_WRITE_SAME)
> > +				generic_make_request(md_writesame_setup(mddev,
> > +								split));
> > +			else
> > +				generic_make_request(split);
> >  		}
> >  	} while (split != bio);
> >  	return;
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 13020e5..7354f0b 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -312,6 +312,51 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
> >  	return BLK_QC_T_NONE;
> >  }
> >  
> > +struct md_writesame_data {
> > +	struct bio *orig_bio;
> > +	struct mddev *mddev;
> > +	struct bio cloned_bio;
> > +};
> > +
> > +static void md_writesame_endio(struct bio *bio)
> > +{
> > +	struct md_writesame_data *data = bio->bi_private;
> > +
> > +	if (bio->bi_error == -EREMOTEIO &&
> > +	    !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
> > +		data->mddev->queue->limits.max_write_same_sectors = 0;
> 
> What would be *really* nice is if a block device could send a
> reconfigure message to its 'holder' (bd_holder).  This could include
> device size changes and, for this, changes to max_write_same_sectors.
> There are probably other changes that can usefully be propagated.
> 
> But for this patch, wouldn't it be easier, and maybe more efficient, to
> do
> 
>   if (bio_op(split) == REQ_OP_WRITE_SAME &&
>       !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
>            mddev->queue->limits.max_write_same_sectors = 0;
>   generic_make_request(split);
> 
> ???
> If there is some reason that can't work, then the patch as it stands
> look OK to me.

So we don't disable writesame in the first IO error and do it until a new
writesame comes? Good idea and much simpler! Let me check if it works.

Thanks,
Shaohua