From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: dm: Fix report zone remapping
Date: Sat, 6 Oct 2018 01:24:43 +0000 [thread overview]
Message-ID: <BN3PR0401MB1640B2763051334D011B0111E7E40@BN3PR0401MB1640.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20181005182948.GA5628@redhat.com
Mike,=0A=
=0A=
On 2018/10/06 3:29, Mike Snitzer wrote:=0A=
> On Fri, Oct 05 2018 at 6:34am -0400,=0A=
> Damien Le Moal <damien.lemoal@wdc.com> wrote:=0A=
> =0A=
>> If dm-linear or dm-flakey have targets on top of a partition of a zoned=
=0A=
>> block device, remapping of the start sector and write pointer position=
=0A=
>> of the zones reported by a report zones BIO must be modified to not=0A=
>> only account for the target table entry mapping, but also to account=0A=
>> for the partition first sector. This start sector must be substracted=0A=
>> to the start sector of all zones reported. The write pointer position=0A=
>> of sequential zones must also be reduced by this offset.=0A=
>>=0A=
>> Since there is no easy way to access the underlying bdev of the target=
=0A=
>> table entry from the dm_target or dm_target_io pointers, modify the=0A=
>> interface of the function dm_remap_zone_report() to allow a target to=0A=
>> pass the partition block device starting sector (offset).=0A=
>>=0A=
>> Fixes: 10999307c14e ("dm: introduce dm_remap_zone_report()")=0A=
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>=0A=
>> Cc: <stable@vger.kernel.org>=0A=
> =0A=
> [dropping the actual stable cc from the mail, please don't cc stable in=
=0A=
> the mail header, could be that git-send-email just pulled it in but...]=
=0A=
=0A=
My apologies about that. It was indeed git-send-email. Will fix that.=0A=
=0A=
> This needs a different fix. There should be absolutely no need to pass=
=0A=
> in a start offset for a device that is already supposed to be described=
=0A=
> within the 'struct bio' passed to dm_remap_zone_report().=0A=
> =0A=
> Why can't you just use the bio->bi_bdev ? Err, since commit=0A=
> 74d46992e0d9d, I mean bio->bi_disk to get the start sector?=0A=
> =0A=
> You should be able to get it from:=0A=
> =0A=
> struct hd_struct *part =3D disk_get_part(bio->bi_disk, bio->bi_partno);=
=0A=
> part->start_sect;=0A=
> disk_put_part(part);=0A=
> =0A=
> or:=0A=
> =0A=
> struct block_device *bdev =3D bdget_disk(bio->bi_disk, bio->bi_partno);=
=0A=
> get_start_sect(fc->dev->bdev);=0A=
> bdput(bdev);=0A=
=0A=
I am afraid this does not work. The original bio disk is of course /dev/dm-=
xx so=0A=
not the partition (partno =3D=3D 0), and the mapped bio passed as argument =
to the=0A=
remap zone function (clone of the orig bio) is pointing to the entire under=
lying=0A=
disk, not the partition disk used in dmsetup. bi_partno is 0 for both BIOs.=
=0A=
Which seems weird, __bio_clone_fast() does copy the disk and part number as=
is.=0A=
The disk is of course correctly changed from the orig dm disk to the actual=
=0A=
physical disk in the clone, but the partition number stays the same. In the=
end,=0A=
only the target has the correct bdev (disk+partno) information it seems.=0A=
=0A=
It looks like the partno gets lost, which does not seem correct. I must be=
=0A=
missing something, but I fail to see what it is. Any suggestion ?=0A=
=0A=
=0A=
> =0A=
> Mike=0A=
> =0A=
> =0A=
>> ---=0A=
>> drivers/md/dm-flakey.c | 3 ++-=0A=
>> drivers/md/dm-linear.c | 3 ++-=0A=
>> drivers/md/dm.c | 16 ++++++++++++----=0A=
>> include/linux/device-mapper.h | 2 +-=0A=
>> 4 files changed, 17 insertions(+), 7 deletions(-)=0A=
>>=0A=
>> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c=0A=
>> index 21d126a5078c..c68c2e9cde6b 100644=0A=
>> --- a/drivers/md/dm-flakey.c=0A=
>> +++ b/drivers/md/dm-flakey.c=0A=
>> @@ -381,7 +381,8 @@ static int flakey_end_io(struct dm_target *ti, struc=
t bio *bio,=0A=
>> return DM_ENDIO_DONE;=0A=
>> =0A=
>> if (bio_op(bio) =3D=3D REQ_OP_ZONE_REPORT) {=0A=
>> - dm_remap_zone_report(ti, bio, fc->start);=0A=
>> + dm_remap_zone_report(ti, bio, get_start_sect(fc->dev->bdev),=0A=
>> + fc->start);=0A=
>> return DM_ENDIO_DONE;=0A=
>> }=0A=
>> =0A=
>> diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c=0A=
>> index d10964d41fd7..f8cd367abbf6 100644=0A=
>> --- a/drivers/md/dm-linear.c=0A=
>> +++ b/drivers/md/dm-linear.c=0A=
>> @@ -108,7 +108,8 @@ static int linear_end_io(struct dm_target *ti, struc=
t bio *bio,=0A=
>> struct linear_c *lc =3D ti->private;=0A=
>> =0A=
>> if (!*error && bio_op(bio) =3D=3D REQ_OP_ZONE_REPORT)=0A=
>> - dm_remap_zone_report(ti, bio, lc->start);=0A=
>> + dm_remap_zone_report(ti, bio, get_start_sect(lc->dev->bdev),=0A=
>> + lc->start);=0A=
>> =0A=
>> return DM_ENDIO_DONE;=0A=
>> }=0A=
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c=0A=
>> index 20f7e4ef5342..ffd8544eedd8 100644=0A=
>> --- a/drivers/md/dm.c=0A=
>> +++ b/drivers/md/dm.c=0A=
>> @@ -1162,7 +1162,8 @@ EXPORT_SYMBOL_GPL(dm_accept_partial_bio);=0A=
>> * REQ_OP_ZONE_REPORT bio to remap the zone descriptors obtained=0A=
>> * from the target device mapping to the dm device.=0A=
>> */=0A=
>> -void dm_remap_zone_report(struct dm_target *ti, struct bio *bio, sector=
_t start)=0A=
>> +void dm_remap_zone_report(struct dm_target *ti, struct bio *bio,=0A=
>> + sector_t dev_offset, sector_t start)=0A=
>> {=0A=
>> #ifdef CONFIG_BLK_DEV_ZONED=0A=
>> struct dm_target_io *tio =3D container_of(bio, struct dm_target_io, cl=
one);=0A=
>> @@ -1195,18 +1196,25 @@ void dm_remap_zone_report(struct dm_target *ti, =
struct bio *bio, sector_t start)=0A=
>> /* Set zones start sector */=0A=
>> while (hdr->nr_zones && ofst < bvec.bv_len) {=0A=
>> zone =3D addr + ofst;=0A=
>> + zone->start -=3D dev_offset;=0A=
>> if (zone->start >=3D start + ti->len) {=0A=
>> hdr->nr_zones =3D 0;=0A=
>> break;=0A=
>> }=0A=
>> zone->start =3D zone->start + ti->begin - start;=0A=
>> if (zone->type !=3D BLK_ZONE_TYPE_CONVENTIONAL) {=0A=
>> - if (zone->cond =3D=3D BLK_ZONE_COND_FULL)=0A=
>> + switch (zone->cond) {=0A=
>> + case BLK_ZONE_COND_FULL:=0A=
>> zone->wp =3D zone->start + zone->len;=0A=
>> - else if (zone->cond =3D=3D BLK_ZONE_COND_EMPTY)=0A=
>> + break;=0A=
>> + case BLK_ZONE_COND_EMPTY:=0A=
>> zone->wp =3D zone->start;=0A=
>> - else=0A=
>> + break;=0A=
>> + default:=0A=
>> + zone->wp -=3D dev_offset;=0A=
>> zone->wp =3D zone->wp + ti->begin - start;=0A=
>> + break;=0A=
>> + }=0A=
>> }=0A=
>> ofst +=3D sizeof(struct blk_zone);=0A=
>> hdr->nr_zones--;=0A=
>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper=
.h=0A=
>> index 6fb0808e87c8..22c1924c0ee8 100644=0A=
>> --- a/include/linux/device-mapper.h=0A=
>> +++ b/include/linux/device-mapper.h=0A=
>> @@ -421,7 +421,7 @@ int dm_suspended(struct dm_target *ti);=0A=
>> int dm_noflush_suspending(struct dm_target *ti);=0A=
>> void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors);=0A=
>> void dm_remap_zone_report(struct dm_target *ti, struct bio *bio,=0A=
>> - sector_t start);=0A=
>> + sector_t dev_offset, sector_t start);=0A=
>> union map_info *dm_get_rq_mapinfo(struct request *rq);=0A=
>> =0A=
>> struct queue_limits *dm_get_queue_limits(struct mapped_device *md);=0A=
>> -- =0A=
>> 2.17.1=0A=
>>=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=
WARNING: multiple messages have this Message-ID (diff)
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"dm-devel@redhat.com" <dm-devel@redhat.com>
Subject: Re: dm: Fix report zone remapping
Date: Sat, 6 Oct 2018 01:24:43 +0000 [thread overview]
Message-ID: <BN3PR0401MB1640B2763051334D011B0111E7E40@BN3PR0401MB1640.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20181005182948.GA5628@redhat.com
Mike,
On 2018/10/06 3:29, Mike Snitzer wrote:
> On Fri, Oct 05 2018 at 6:34am -0400,
> Damien Le Moal <damien.lemoal@wdc.com> wrote:
>
>> If dm-linear or dm-flakey have targets on top of a partition of a zoned
>> block device, remapping of the start sector and write pointer position
>> of the zones reported by a report zones BIO must be modified to not
>> only account for the target table entry mapping, but also to account
>> for the partition first sector. This start sector must be substracted
>> to the start sector of all zones reported. The write pointer position
>> of sequential zones must also be reduced by this offset.
>>
>> Since there is no easy way to access the underlying bdev of the target
>> table entry from the dm_target or dm_target_io pointers, modify the
>> interface of the function dm_remap_zone_report() to allow a target to
>> pass the partition block device starting sector (offset).
>>
>> Fixes: 10999307c14e ("dm: introduce dm_remap_zone_report()")
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> Cc: <stable@vger.kernel.org>
>
> [dropping the actual stable cc from the mail, please don't cc stable in
> the mail header, could be that git-send-email just pulled it in but...]
My apologies about that. It was indeed git-send-email. Will fix that.
> This needs a different fix. There should be absolutely no need to pass
> in a start offset for a device that is already supposed to be described
> within the 'struct bio' passed to dm_remap_zone_report().
>
> Why can't you just use the bio->bi_bdev ? Err, since commit
> 74d46992e0d9d, I mean bio->bi_disk to get the start sector?
>
> You should be able to get it from:
>
> struct hd_struct *part = disk_get_part(bio->bi_disk, bio->bi_partno);
> part->start_sect;
> disk_put_part(part);
>
> or:
>
> struct block_device *bdev = bdget_disk(bio->bi_disk, bio->bi_partno);
> get_start_sect(fc->dev->bdev);
> bdput(bdev);
I am afraid this does not work. The original bio disk is of course /dev/dm-xx so
not the partition (partno == 0), and the mapped bio passed as argument to the
remap zone function (clone of the orig bio) is pointing to the entire underlying
disk, not the partition disk used in dmsetup. bi_partno is 0 for both BIOs.
Which seems weird, __bio_clone_fast() does copy the disk and part number as is.
The disk is of course correctly changed from the orig dm disk to the actual
physical disk in the clone, but the partition number stays the same. In the end,
only the target has the correct bdev (disk+partno) information it seems.
It looks like the partno gets lost, which does not seem correct. I must be
missing something, but I fail to see what it is. Any suggestion ?
>
> Mike
>
>
>> ---
>> drivers/md/dm-flakey.c | 3 ++-
>> drivers/md/dm-linear.c | 3 ++-
>> drivers/md/dm.c | 16 ++++++++++++----
>> include/linux/device-mapper.h | 2 +-
>> 4 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
>> index 21d126a5078c..c68c2e9cde6b 100644
>> --- a/drivers/md/dm-flakey.c
>> +++ b/drivers/md/dm-flakey.c
>> @@ -381,7 +381,8 @@ static int flakey_end_io(struct dm_target *ti, struct bio *bio,
>> return DM_ENDIO_DONE;
>>
>> if (bio_op(bio) == REQ_OP_ZONE_REPORT) {
>> - dm_remap_zone_report(ti, bio, fc->start);
>> + dm_remap_zone_report(ti, bio, get_start_sect(fc->dev->bdev),
>> + fc->start);
>> return DM_ENDIO_DONE;
>> }
>>
>> diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
>> index d10964d41fd7..f8cd367abbf6 100644
>> --- a/drivers/md/dm-linear.c
>> +++ b/drivers/md/dm-linear.c
>> @@ -108,7 +108,8 @@ static int linear_end_io(struct dm_target *ti, struct bio *bio,
>> struct linear_c *lc = ti->private;
>>
>> if (!*error && bio_op(bio) == REQ_OP_ZONE_REPORT)
>> - dm_remap_zone_report(ti, bio, lc->start);
>> + dm_remap_zone_report(ti, bio, get_start_sect(lc->dev->bdev),
>> + lc->start);
>>
>> return DM_ENDIO_DONE;
>> }
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 20f7e4ef5342..ffd8544eedd8 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -1162,7 +1162,8 @@ EXPORT_SYMBOL_GPL(dm_accept_partial_bio);
>> * REQ_OP_ZONE_REPORT bio to remap the zone descriptors obtained
>> * from the target device mapping to the dm device.
>> */
>> -void dm_remap_zone_report(struct dm_target *ti, struct bio *bio, sector_t start)
>> +void dm_remap_zone_report(struct dm_target *ti, struct bio *bio,
>> + sector_t dev_offset, sector_t start)
>> {
>> #ifdef CONFIG_BLK_DEV_ZONED
>> struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
>> @@ -1195,18 +1196,25 @@ void dm_remap_zone_report(struct dm_target *ti, struct bio *bio, sector_t start)
>> /* Set zones start sector */
>> while (hdr->nr_zones && ofst < bvec.bv_len) {
>> zone = addr + ofst;
>> + zone->start -= dev_offset;
>> if (zone->start >= start + ti->len) {
>> hdr->nr_zones = 0;
>> break;
>> }
>> zone->start = zone->start + ti->begin - start;
>> if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) {
>> - if (zone->cond == BLK_ZONE_COND_FULL)
>> + switch (zone->cond) {
>> + case BLK_ZONE_COND_FULL:
>> zone->wp = zone->start + zone->len;
>> - else if (zone->cond == BLK_ZONE_COND_EMPTY)
>> + break;
>> + case BLK_ZONE_COND_EMPTY:
>> zone->wp = zone->start;
>> - else
>> + break;
>> + default:
>> + zone->wp -= dev_offset;
>> zone->wp = zone->wp + ti->begin - start;
>> + break;
>> + }
>> }
>> ofst += sizeof(struct blk_zone);
>> hdr->nr_zones--;
>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
>> index 6fb0808e87c8..22c1924c0ee8 100644
>> --- a/include/linux/device-mapper.h
>> +++ b/include/linux/device-mapper.h
>> @@ -421,7 +421,7 @@ int dm_suspended(struct dm_target *ti);
>> int dm_noflush_suspending(struct dm_target *ti);
>> void dm_accept_partial_bio(struct bio *bio, unsigned n_sectors);
>> void dm_remap_zone_report(struct dm_target *ti, struct bio *bio,
>> - sector_t start);
>> + sector_t dev_offset, sector_t start);
>> union map_info *dm_get_rq_mapinfo(struct request *rq);
>>
>> struct queue_limits *dm_get_queue_limits(struct mapped_device *md);
>> --
>> 2.17.1
>>
>
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2018-10-06 8:26 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-05 10:34 [PATCH] dm: Fix report zone remapping Damien Le Moal
2018-10-05 18:29 ` Mike Snitzer
2018-10-05 18:29 ` Mike Snitzer
2018-10-06 1:24 ` Damien Le Moal [this message]
2018-10-06 1:24 ` Damien Le Moal
2018-10-06 3:49 ` Mike Snitzer
2018-10-06 3:49 ` Mike Snitzer
2018-10-06 4:03 ` Damien Le Moal
2018-10-06 4:03 ` Damien Le Moal
2018-10-06 5:32 ` Mike Snitzer
2018-10-06 5:32 ` Mike Snitzer
2018-10-06 7:37 ` Damien Le Moal
2018-10-06 7:37 ` Damien Le Moal
2018-10-08 16:25 ` Mike Snitzer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN3PR0401MB1640B2763051334D011B0111E7E40@BN3PR0401MB1640.namprd04.prod.outlook.com \
--to=damien.lemoal@wdc.com \
--cc=dm-devel@redhat.com \
--cc=linux-block@vger.kernel.org \
--cc=snitzer@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.