Skip to contentSkip to conversation

fix writesame

PATCH In review Shaohua Li 3 participants 5 messages Feb 13, 2017 – Feb 14, 2017 MD RAID

lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

cover fix writesame

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(-)
Signature · 1 line
2.9.3
author
#1 lore raw

PATCH 1/2 md: disable WRITE SAME if it fails for linear/raid0

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

---
 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(-)
drivers/md/linear.c +5 -1
index 26a73b2..bebc834 100644
@@ -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;
drivers/md/md.c +45 -0

51 lines not on this page · raw message

drivers/md/md.h +2 -0
index 2a51403..5c983e9 100644
@@ -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 */
drivers/md/raid0.c +5 -1
index 848365d..e38636e 100644
@@ -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);
 }
Signature · 1 line
2.9.3
#2 lore raw

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

Quoted 42 lines · 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

---
 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(-)
drivers/md/linear.c +5 -1
index 26a73b2..bebc834 100644
@@ -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;
drivers/md/md.c +3 -0
index 13020e5..7354f0b 100644
@@ -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;

drivers/md/md.c static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio) · line 327

Quoted 10 lines
drivers/md/md.c +10 -0
+	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

#3 lore raw

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

Quoted 63 lines · Neil Brown
Quoted 42 lines · 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

---
 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(-)
drivers/md/linear.c +5 -1
index 26a73b2..bebc834 100644
@@ -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;
drivers/md/md.c +3 -0
index 13020e5..7354f0b 100644
@@ -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;

drivers/md/md.c static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio) · line 327

Quoted 10 lines
drivers/md/md.c +10 -0
+	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

Quoted 8 lines
  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

author
#4 lore raw

PATCH 2/2 md/multipath: disable WRITE SAME if it fails for multipath

In reply to Shaohua Li

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

---
 drivers/md/multipath.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/md/multipath.c +4 -0
index aa8c4e5c..5ee9579 100644
@@ -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)) {
Signature · 1 line
2.9.3