[PATCH 0/2] Two fixes for device-mapper with REQ_OP_ZONE_APPEND

[PATCH 0/2] Two fixes for device-mapper with REQ_OP_ZONE_APPEND

[Johannes Thumshirn]

Two small fixes for issuing REQ_OP_ZONE_APPEND writes to device-mapper. The
first one is an issue reported by Naohiro and fixes a write failure. For
number two I'm not certain if it's the correct fix, hence the RFC tag.

Johannes Thumshirn (2):
  dm: update original bio sector on Zone Append
  dm: don't try to split REQ_OP_ZONE_APPEND bios

 drivers/md/dm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.26.2

[PATCH 1/2] dm: update original bio sector on Zone Append

[Johannes Thumshirn]

Naohiro reported that issuing zone-append bios to a zoned block device
underneath a dm-linear device does not work as expected.

This because we forgot to reverse-map the sector the device wrote to the
original bio.

For zone-append bios, get the offset in the zone of the written sector
from the clone bio and add that to the original bio's sector position.

Reported-by: Naohiro Aota <Naohiro.Aota@wdc.com>
Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND")
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/md/dm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 109e81f33edb..058c34abe9d1 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1009,6 +1009,7 @@ static void clone_endio(struct bio *bio)
 	struct dm_io *io = tio->io;
 	struct mapped_device *md = tio->io->md;
 	dm_endio_fn endio = tio->ti->type->end_io;
+	struct bio *orig_bio = io->orig_bio;
 
 	if (unlikely(error == BLK_STS_TARGET) && md->type != DM_TYPE_NVME_BIO_BASED) {
 		if (bio_op(bio) == REQ_OP_DISCARD &&
@@ -1022,6 +1023,18 @@ static void clone_endio(struct bio *bio)
 			disable_write_zeroes(md);
 	}
 
+	/*
+	 * for zone-append bios get offset in zone of the written sector and add
+	 * that to the original bio sector pos.
+	 */
+	if (bio_op(orig_bio) == REQ_OP_ZONE_APPEND) {
+		sector_t written_sector = bio->bi_iter.bi_sector;
+		struct request_queue *q = orig_bio->bi_disk->queue;
+		u64 mask = (u64)blk_queue_zone_sectors(q) - 1;
+
+		orig_bio->bi_iter.bi_sector += written_sector & mask;
+	}
+
 	if (endio) {
 		int r = endio(tio->ti, bio, &error);
 		switch (r) {
-- 
2.26.2

[RFC PATCH 2/2] dm: don't try to split REQ_OP_ZONE_APPEND bios

[Johannes Thumshirn]

REQ_OP_ZONE_APPEND bios cannot be split so return EIO if we can't fit it
into one IO.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/md/dm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 058c34abe9d1..c720a7e3269a 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1609,6 +1609,9 @@ static int __split_and_process_non_flush(struct clone_info *ci)
 
 	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
 
+	if (bio_op(ci->bio) == REQ_OP_ZONE_APPEND && len < ci->sector_count)
+		return -EIO;
+
 	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
 	if (r < 0)
 		return r;
-- 
2.26.2

Re: [PATCH 1/2] dm: update original bio sector on Zone Append

[Damien Le Moal]

On 2020/06/19 15:59, Johannes Thumshirn wrote:
> Naohiro reported that issuing zone-append bios to a zoned block device
> underneath a dm-linear device does not work as expected.
> 
> This because we forgot to reverse-map the sector the device wrote to the
> original bio.
> 
> For zone-append bios, get the offset in the zone of the written sector
> from the clone bio and add that to the original bio's sector position.
> 
> Reported-by: Naohiro Aota <Naohiro.Aota@wdc.com>
> Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND")
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  drivers/md/dm.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 109e81f33edb..058c34abe9d1 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1009,6 +1009,7 @@ static void clone_endio(struct bio *bio)
>  	struct dm_io *io = tio->io;
>  	struct mapped_device *md = tio->io->md;
>  	dm_endio_fn endio = tio->ti->type->end_io;
> +	struct bio *orig_bio = io->orig_bio;
>  
>  	if (unlikely(error == BLK_STS_TARGET) && md->type != DM_TYPE_NVME_BIO_BASED) {
>  		if (bio_op(bio) == REQ_OP_DISCARD &&
> @@ -1022,6 +1023,18 @@ static void clone_endio(struct bio *bio)
>  			disable_write_zeroes(md);
>  	}
>  
> +	/*
> +	 * for zone-append bios get offset in zone of the written sector and add
> +	 * that to the original bio sector pos.
> +	 */
> +	if (bio_op(orig_bio) == REQ_OP_ZONE_APPEND) {
> +		sector_t written_sector = bio->bi_iter.bi_sector;
> +		struct request_queue *q = orig_bio->bi_disk->queue;
> +		u64 mask = (u64)blk_queue_zone_sectors(q) - 1;
> +
> +		orig_bio->bi_iter.bi_sector += written_sector & mask;
> +	}
> +
>  	if (endio) {
>  		int r = endio(tio->ti, bio, &error);
>  		switch (r) {
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH 2/2] dm: don't try to split REQ_OP_ZONE_APPEND bios

[Damien Le Moal]

On 2020/06/19 15:59, Johannes Thumshirn wrote:
> REQ_OP_ZONE_APPEND bios cannot be split so return EIO if we can't fit it
> into one IO.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  drivers/md/dm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 058c34abe9d1..c720a7e3269a 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1609,6 +1609,9 @@ static int __split_and_process_non_flush(struct clone_info *ci)
>  
>  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
>  
> +	if (bio_op(ci->bio) == REQ_OP_ZONE_APPEND && len < ci->sector_count)
> +		return -EIO;
> +
>  	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
>  	if (r < 0)
>  		return r;
> 

I think this is OK. The stacked max_zone_append_sectors limit should have
prevented that to happen  in the first place I think, but better safe than sorry.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH 2/2] dm: don't try to split REQ_OP_ZONE_APPEND bios

[Mike Snitzer]

On Fri, Jun 19 2020 at  3:54am -0400,
Damien Le Moal <Damien.LeMoal@wdc.com> wrote:

> On 2020/06/19 15:59, Johannes Thumshirn wrote:
> > REQ_OP_ZONE_APPEND bios cannot be split so return EIO if we can't fit it
> > into one IO.
> > 
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > ---
> >  drivers/md/dm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 058c34abe9d1..c720a7e3269a 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1609,6 +1609,9 @@ static int __split_and_process_non_flush(struct clone_info *ci)
> >  
> >  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
> >  
> > +	if (bio_op(ci->bio) == REQ_OP_ZONE_APPEND && len < ci->sector_count)
> > +		return -EIO;
> > +
> >  	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
> >  	if (r < 0)
> >  		return r;
> > 
> 
> I think this is OK. The stacked max_zone_append_sectors limit should have
> prevented that to happen  in the first place I think, but better safe than sorry.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

If stacked max_zone_append_sectors limit should prevent it then I'd
rather not sprinkle more zoned specific checks in DM core.

Thanks,
Mike

Re: [RFC PATCH 2/2] dm: don't try to split REQ_OP_ZONE_APPEND bios

[Damien Le Moal]

On 2020/06/20 1:27, Mike Snitzer wrote:
> On Fri, Jun 19 2020 at  3:54am -0400,
> Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
> 
>> On 2020/06/19 15:59, Johannes Thumshirn wrote:
>>> REQ_OP_ZONE_APPEND bios cannot be split so return EIO if we can't fit it
>>> into one IO.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> ---
>>>  drivers/md/dm.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>>> index 058c34abe9d1..c720a7e3269a 100644
>>> --- a/drivers/md/dm.c
>>> +++ b/drivers/md/dm.c
>>> @@ -1609,6 +1609,9 @@ static int __split_and_process_non_flush(struct clone_info *ci)
>>>  
>>>  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
>>>  
>>> +	if (bio_op(ci->bio) == REQ_OP_ZONE_APPEND && len < ci->sector_count)
>>> +		return -EIO;
>>> +
>>>  	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
>>>  	if (r < 0)
>>>  		return r;
>>>
>>
>> I think this is OK. The stacked max_zone_append_sectors limit should have
>> prevented that to happen  in the first place I think, but better safe than sorry.
>>
>> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> 
> If stacked max_zone_append_sectors limit should prevent it then I'd
> rather not sprinkle more zoned specific checks in DM core.

Mike,

Just to be extra sure, I checked this again. Since for zoned block devices the
mapping of a target must be zoned aligned and a zone append command is always
fully contained within a zone, we do not need this check. The stacked limits and
submit_bio() code will fail a zone append command that is too large or that
spans zones before we get here.

That is of course assuming that the target does not expose zones that are mapped
using multiple chunks from different devices. There is currently no target doing
that, so this is OK. We can safely drop this patch.

Thanks.

> 
> Thanks,
> Mike
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH 2/2] dm: don't try to split REQ_OP_ZONE_APPEND bios

[Johannes Thumshirn]

On 22/06/2020 02:27, Damien Le Moal wrote:
[...]
> Just to be extra sure, I checked this again. Since for zoned block devices the
> mapping of a target must be zoned aligned and a zone append command is always
> fully contained within a zone, we do not need this check. The stacked limits and
> submit_bio() code will fail a zone append command that is too large or that
> spans zones before we get here.
> 
> That is of course assuming that the target does not expose zones that are mapped
> using multiple chunks from different devices. There is currently no target doing
> that, so this is OK. We can safely drop this patch.

Yeah I think we can drop that one.