dm linear: disable WRITE SAME if it fails

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.

this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

Before this patch:

dm linear device(dm-20): ocfs2-fs;
multipath device(dm-6);
scsi device(sdt,sdu);

After a WRITE SAME bio fails, the DM linear device does not disable the WRITE SAME, although the underlying device(DM multipath device) has disabled WRITE SAME.

cat /sys/block/dm-20/queue/write_same_max_bytes
33553920
cat /sys/block/dm-6/queue/write_same_max_bytes
0
cat /sys/block/sdt/queue/write_same_max_bytes
0
cat /sys/block/sdu/queue/write_same_max_bytes
0

After this patch:

dm linear device(dm-20): ocfs2-fs;
multipath device(dm-6);
scsi device(sdt,sdu);

After a WRITE SAME bio fails, the underlying device(DM multipath device) disables WRITE SAME and then the DM linear device also disables WRITE SAME.

cat /sys/block/dm-20/queue/write_same_max_bytes
0
cat /sys/block/dm-6/queue/write_same_max_bytes
0
cat /sys/block/sdt/queue/write_same_max_bytes
0
cat /sys/block/sdu/queue/write_same_max_bytes
0

Signed-off-by alex.chen <[email protected]>

---
 drivers/md/dm-linear.c | 26 +++++++++++++++++++++++++-
 drivers/md/dm.c        |  2 +-
 2 files changed, 26 insertions(+), 2 deletions(-)

drivers/md/dm-linear.c +23 -0

index 53e848c..eaf86a5 100644
@@ -96,6 +96,29 @@ static int linear_map(struct dm_target *ti, struct
 bio *bio)
 	return DM_MAPIO_REMAPPED;
 }
 
+static int linear_end_io(struct dm_target *ti, struct bio *bio, int error)
+{
+	struct linear_c *lc = ti->private;
+	struct block_device *bdev;
+	struct queue_limits *limits;
+	struct queue_limits *bdev_limits;
+
+	if (!error)
+		return error;
+
+	if (unlikely(bio->bi_rw & REQ_WRITE_SAME)) {
+		bdev = lc->dev->bdev;
+		bdev_limits = &bdev->bd_disk->queue->limits;
+
+		limits = dm_get_queue_limits(dm_table_get_md(ti->table));
+
+		if ((!bdev_limits->max_write_same_sectors) &&
+				limits->max_write_same_sectors)
+			limits->max_write_same_sectors = 0;
+	}
+
+	return error;
+}
 static void linear_status(struct dm_target *ti, status_type_t type,
 			  unsigned status_flags, char *result, unsigned maxlen)

{

drivers/md/dm-linear.c +2 -1

@@ -155,11 +178,12 @@ static int linear_iterate_devices(struct dm_target
 *ti,
 
 static struct target_type linear_target = {
 	.name   = "linear",
-	.version = {1, 2, 1},
+	.version = {1, 2, 2},
 	.module = THIS_MODULE,
 	.ctr    = linear_ctr,
 	.dtr    = linear_dtr,
 	.map    = linear_map,
+	.end_io = linear_end_io,
 	.status = linear_status,
 	.ioctl  = linear_ioctl,

drivers/md/dm-linear.c +0 -0

 	.merge  = linear_merge,

drivers/md/dm.c +1 -1

index 8c53b09..4337025 100644
@@ -770,7 +770,7 @@ static void clone_endio(struct bio *bio, int error)
 
 	if (endio) {
 		r = endio(tio->ti, bio, error);
-		if (r < 0 || r == DM_ENDIO_REQUEUE)
+		if (r <= 0 || r == DM_ENDIO_REQUEUE)
 			/*
 			 * error and requeue request are handled
 			 * in dec_pending().

Who can help me review my patch? ths.

this patch adds DM linear end_io method

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

Quoted 7 lines · alex chen

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

How does your patch address striped targets?

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Quoted 11 lines · Alasdair G Kergon

Quoted 7 lines · alex chen

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

How does your patch address striped targets?

Quoted 2 lines

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Alex,

I've implemented what Alasdair and I have been suggesting. Can you please test this untested patch?

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

"Mike" == Mike Snitzer <[email protected]> writes:

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

"Mike" == Mike Snitzer <[email protected]> writes:

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Mike> FYI, I pushed a revised version of this patch to linux-next,

I haven't tested the patch but I definitely agree with this approach.

Acked-by Martin K. Petersen <[email protected]>

Quoted 9 lines · Alasdair G Kergon

Quoted 7 lines · alex chen

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

Quoted 4 lines

How does your patch address striped targets?

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Alex,

I've implemented what Alasdair and I have been suggesting. Can you please test this untested patch?

Thanks,
Mike

---
 drivers/md/dm-mpath.c |   11 +----------
 drivers/md/dm.c       |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

drivers/md/dm-mpath.c +1 -10

index fa0f6cb..9eb6dfd 100644
@@ -1240,17 +1240,8 @@ static int do_end_io(struct multipath *m, struct request *clone,
 	if (!error && !clone->errors)
 		return 0;	/* I/O complete */
 
-	if (noretry_error(error)) {
-		if ((clone->cmd_flags & REQ_WRITE_SAME) &&
-		    !clone->q->limits.max_write_same_sectors) {
-			struct queue_limits *limits;
-
-			/* device doesn't really support WRITE SAME, disable it */
-			limits = dm_get_queue_limits(dm_table_get_md(m->ti->table));
-			limits->max_write_same_sectors = 0;
-		}
+	if (noretry_error(error))
 		return error;
-	}
 
 	if (mpio->pgpath)
 		fail_path(mpio->pgpath);

drivers/md/dm.c +16 -0

index 97940fc..c37581c 100644
@@ -755,6 +755,14 @@ static void dec_pending(struct dm_io *io, int error)
 	}
 }
 
+static void disable_write_same(struct mapped_device *md)
+{
+	struct queue_limits *limits = dm_get_queue_limits(md);
+
+	/* device doesn't really support WRITE SAME, disable it */
+	limits->max_write_same_sectors = 0;
+}
+
 static void clone_endio(struct bio *bio, int error)
 {
 	int r = 0;

@@ -783,6 +791,10 @@ static void clone_endio(struct bio *bio, int error)
 		}
 	}
 
+	if (r < 0 && (bio->bi_rw & REQ_WRITE_SAME) &&
+	    !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors)
+		disable_write_same(md);
+
 	free_tio(md, tio);
 	dec_pending(io, error);
 }

@@ -977,6 +989,10 @@ static void dm_done(struct request *clone, int error, bool mapped)
 			r = rq_end_io(tio->ti, clone, error, &tio->info);
 	}
 
+	if (r < 0 && (clone->cmd_flags & REQ_WRITE_SAME) &&
+	    !clone->q->limits.max_write_same_sectors)
+		disable_write_same(tio->md);
+
 	if (r <= 0)
 		/* The target wants to complete the I/O */
 		dm_end_request(clone, r);

.

Quoted 17 lines · Mike Snitzer

Quoted 9 lines · Alasdair G Kergon

Quoted 7 lines · alex chen

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

Quoted 4 lines

How does your patch address striped targets?

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Quoted 8 lines

Alex,

I've implemented what Alasdair and I have been suggesting. Can you please test this untested patch?

Thanks,
Mike

I tested this patch, it is OK.
Test cases is as follow:
CASE 1:
dm linear device(dm-23): ocfs2-fs;
multipath device(dm-9);
scsi device(sdr,sdv);

After WRITE SAME IO fails:
linux-MOLybD:~ # cat /sys/block/dm-23/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/dm-9/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/sdr/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/sdv/queue/write_same_max_bytes 0

Result: pass

Tested-by Alex Chen <[email protected]>

Quoted 44 lines · alex chen

Quoted 17 lines · Mike Snitzer

Quoted 9 lines · Alasdair G Kergon

Quoted 7 lines · alex chen

The original commit f84cb8a46a771f36a04a02c61ea635c968ed5f6a("dm mpath:
disable WRITE SAME if it fails") disables WRITE SAME in the DM multipath device if it fails, but when the DM linear device stacks ontop of the multipath device it doesn't help.
this patch adds DM linear end_io method to catch WRITE SAME errors and disables WRITE SAME in the DM linear device's queue_limits if an underlying device disables it.

Quoted 4 lines

How does your patch address striped targets?

Shouldn't this code be taken out of mpath and moved to dm.c and applied to all targets (both bio and rq-based, at least where WRITE SAME is supported)?

Quoted 8 lines

Alex,

I've implemented what Alasdair and I have been suggesting. Can you please test this untested patch?

Thanks,
Mike

I tested this patch, it is OK.
Test cases is as follow:
CASE 1:
dm linear device(dm-23): ocfs2-fs;
multipath device(dm-9);
scsi device(sdr,sdv);

After WRITE SAME IO fails:
linux-MOLybD:~ # cat /sys/block/dm-23/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/dm-9/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/sdr/queue/write_same_max_bytes 0
linux-MOLybD:~ # cat /sys/block/sdv/queue/write_same_max_bytes 0

Quoted 3 lines

Result: pass

Tested-by Alex Chen <[email protected]>

OK, thanks.. FYI, as I sahred before: I updated it slightly to check for the specific -EREMOTEIO error code that SCSI returns for this case, see:

https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7eee4ae2dbb2be0a15a4406718806e48b18ba831

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

.

Quoted 6 lines · Mike Snitzer

OK, thanks.. FYI, as I sahred before: I updated it slightly to check for the specific -EREMOTEIO error code that SCSI returns for this case, see:

https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=7eee4ae2dbb2be0a15a4406718806e48b18ba831

OK，I've tested the new patch, the result was also pass.