| Message ID | 20231023224513.344380-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm: error: Add support for zoned block devices | expand |
Thanks, this looks good to me and fixes the problems I've seen Reviewed-by: Christoph Hellwig <hch@lst.de> Tested-by: Christoph Hellwig <hch@lst.de> FTI, this is the xfstests change we need to use dm-error for zoned devices in xfstests: diff --git a/common/rc b/common/rc index 741579af..9e07d79d 100644 --- a/common/rc +++ b/common/rc @@ -2174,12 +2174,10 @@ _require_dm_target() _notrun "This test requires dm $target support" fi - # dm-error cannot handle the zone information - # # dm-snapshot and dm-thin-pool cannot ensure sequential writes on # the backing device case $target in - error|snapshot|thin-pool) + snapshot|thin-pool) _require_non_zoned_device ${SCRATCH_DEV} ;; esac
Looks good to me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Tue, Oct 24, 2023 at 07:45:13AM +0900, Damien Le Moal wrote: > dm-error is used in several test cases in the xfstests test suite to > check the handling of IO errors in file syatems. However, with several ^ systems > file systems getting native support for zoned block devices (e.g. btrfs > and f2fs), dm-error lack of zoned block device support creates problems > as the file system attempt executing zone commands (e.g. a zone append > operation) against a dm-error non-zoned block device, which causes > various issues in the block layer (e.g. WARN_ON triggers). > > This patch adds supports for zoned block devices to dm-error, allowing > an error table to be exposed as a zoned block device. This is done by > relying on the first argument passed to dmsetup when creating the device > table: if that first argument is a path to a backing block device, the > dm-error device is created by copying the limits of the backing device, > thus also copying its zone model. This is consistent with how xfstests > creates dm-error devices (always passing the path to the backing device > as the first argument). > > The zone support for dm-error requires the definition of the > report_zones target type method, which is done by introducing the > function io_err_report_zones(). Given that this function fails report > zones operations (similarly to any other command issued to the dm-error > device), dm_set_zones_restrictions() is tweaked to do nothing for a > wildcard target to avoid failing zone revalidation. As the dm-error > target does not implemt the iterate_devices method, ^implement > dm_table_supports_zoned_model() is also changed to return true. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/md/dm-table.c | 3 +++ > drivers/md/dm-target.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > drivers/md/dm-zone.c | 9 +++++++++ > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 37b48f63ae6a..5e4d887063d3 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -1600,6 +1600,9 @@ static bool dm_table_supports_zoned_model(struct dm_table *t, > for (unsigned int i = 0; i < t->num_targets; i++) { > struct dm_target *ti = dm_table_get_target(t, i); > > + if (dm_target_is_wildcard(ti->type)) > + continue; > + This seems tricky to me. Currently, dm-error is the only dm target having DM_TARGET_WILDCARD. But, can we expect that will be so forever? Also, I considered what happens if the backing device is non-zoned one. dm_table_supports_zoned_model() returns true in that case. But, it is still reported as non-zoned as we copy non-zoned queue limits. So, it is OK ... but it is a bit tricky. Instead, how about implementing the .iterate_devices just like linear_iterate_devices? > if (dm_target_supports_zoned_hm(ti->type)) { > if (!ti->type->iterate_devices || > ti->type->iterate_devices(ti, device_not_zoned_model, > diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c > index 27e2992ff249..1bf4ecda3012 100644 > --- a/drivers/md/dm-target.c > +++ b/drivers/md/dm-target.c > @@ -118,6 +118,21 @@ EXPORT_SYMBOL(dm_unregister_target); > */ > static int io_err_ctr(struct dm_target *tt, unsigned int argc, char **args) > { > + struct dm_dev *ddev; > + int ret; > + > + /* > + * If we have an argument, assume it is the path to the target > + * block device we are replacing. In this case, get the device > + * so that we can copy its limits in io_err_io_hints(). > + */ > + if (argc) { > + ret = dm_get_device(tt, args[0], dm_table_get_mode(tt->table), > + &ddev); > + if (ret == 0) > + tt->private = ddev; No need to handle an error case here? Or, I guess it ignores an error for compatibility when non-device argument is specified. Then, I'd like to add a note here. > + } > + > /* > * Return error for discards instead of -EOPNOTSUPP > */ > @@ -129,7 +144,10 @@ static int io_err_ctr(struct dm_target *tt, unsigned int argc, char **args) > > static void io_err_dtr(struct dm_target *tt) > { > - /* empty */ > + struct dm_dev *ddev = tt->private; > + > + if (ddev) > + dm_put_device(tt, ddev); > } > > static int io_err_map(struct dm_target *tt, struct bio *bio) > @@ -149,8 +167,27 @@ static void io_err_release_clone_rq(struct request *clone, > { > } > > +#ifdef CONFIG_BLK_DEV_ZONED > +static int io_err_report_zones(struct dm_target *ti, > + struct dm_report_zones_args *args, unsigned int nr_zones) > +{ > + return -EIO; > +} > +#else > +#define io_err_report_zones NULL > +#endif > + > static void io_err_io_hints(struct dm_target *ti, struct queue_limits *limits) > { > + struct dm_dev *ddev = ti->private; > + > + /* If we have a target device, copy its limits */ > + if (ddev) { > + struct request_queue *q = bdev_get_queue(ddev->bdev); > + > + memcpy(limits, &q->limits, sizeof(*limits)); > + } > + > limits->max_discard_sectors = UINT_MAX; > limits->max_hw_discard_sectors = UINT_MAX; > limits->discard_granularity = 512; > @@ -166,7 +203,7 @@ static long io_err_dax_direct_access(struct dm_target *ti, pgoff_t pgoff, > static struct target_type error_target = { > .name = "error", > .version = {1, 6, 0}, > - .features = DM_TARGET_WILDCARD, > + .features = DM_TARGET_WILDCARD | DM_TARGET_ZONED_HM, > .ctr = io_err_ctr, > .dtr = io_err_dtr, > .map = io_err_map, > @@ -174,6 +211,7 @@ static struct target_type error_target = { > .release_clone_rq = io_err_release_clone_rq, > .io_hints = io_err_io_hints, > .direct_access = io_err_dax_direct_access, > + .report_zones = io_err_report_zones, > }; > > int __init dm_target_init(void) > diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c > index eb9832b22b14..9b77ee05e8dd 100644 > --- a/drivers/md/dm-zone.c > +++ b/drivers/md/dm-zone.c > @@ -297,6 +297,15 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q) > WARN_ON_ONCE(queue_is_mq(q)); > md->disk->nr_zones = bdev_nr_zones(md->disk->part0); > > + /* > + * With dm-error (wildcard target), report zones will fail, so do > + * nothing. dm-error will copy the zones limits itself. > + */ > + if (dm_table_get_wildcard_target(t)) { > + md->nr_zones = md->disk->nr_zones; > + return 0; > + } > + > /* Check if zone append is natively supported */ > if (dm_table_supports_zone_append(t)) { > clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags); > -- > 2.41.0 >
On 10/25/23 14:22, Naohiro Aota wrote: > On Tue, Oct 24, 2023 at 07:45:13AM +0900, Damien Le Moal wrote: >> dm-error is used in several test cases in the xfstests test suite to >> check the handling of IO errors in file syatems. However, with several > ^ systems > >> file systems getting native support for zoned block devices (e.g. btrfs >> and f2fs), dm-error lack of zoned block device support creates problems >> as the file system attempt executing zone commands (e.g. a zone append >> operation) against a dm-error non-zoned block device, which causes >> various issues in the block layer (e.g. WARN_ON triggers). >> >> This patch adds supports for zoned block devices to dm-error, allowing >> an error table to be exposed as a zoned block device. This is done by >> relying on the first argument passed to dmsetup when creating the device >> table: if that first argument is a path to a backing block device, the >> dm-error device is created by copying the limits of the backing device, >> thus also copying its zone model. This is consistent with how xfstests >> creates dm-error devices (always passing the path to the backing device >> as the first argument). >> >> The zone support for dm-error requires the definition of the >> report_zones target type method, which is done by introducing the >> function io_err_report_zones(). Given that this function fails report >> zones operations (similarly to any other command issued to the dm-error >> device), dm_set_zones_restrictions() is tweaked to do nothing for a >> wildcard target to avoid failing zone revalidation. As the dm-error >> target does not implemt the iterate_devices method, > ^implement Thanks. Will fix that. > >> dm_table_supports_zoned_model() is also changed to return true. >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> --- >> drivers/md/dm-table.c | 3 +++ >> drivers/md/dm-target.c | 42 ++++++++++++++++++++++++++++++++++++++++-- >> drivers/md/dm-zone.c | 9 +++++++++ >> 3 files changed, 52 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >> index 37b48f63ae6a..5e4d887063d3 100644 >> --- a/drivers/md/dm-table.c >> +++ b/drivers/md/dm-table.c >> @@ -1600,6 +1600,9 @@ static bool dm_table_supports_zoned_model(struct dm_table *t, >> for (unsigned int i = 0; i < t->num_targets; i++) { >> struct dm_target *ti = dm_table_get_target(t, i); >> >> + if (dm_target_is_wildcard(ti->type)) >> + continue; >> + > > This seems tricky to me. Currently, dm-error is the only dm target having > DM_TARGET_WILDCARD. But, can we expect that will be so forever? Yes, I saw that. Not sure. Mike ? > Also, I considered what happens if the backing device is non-zoned > one. dm_table_supports_zoned_model() returns true in that case. But, it is > still reported as non-zoned as we copy non-zoned queue limits. So, it is > OK ... but it is a bit tricky. Returning true for dm_table_supports_zoned_model() is not an issue. As the name of the function says, this checks that the device table is OK with zoned device but does not force the dm device to be zoned. > Instead, how about implementing the .iterate_devices just like > linear_iterate_devices? Because that would change how dm-error needs to be setup. Currently, there are no arguments needed for the table entry for dm-error. If we define the ->iterate_devices operation, we'll need a backing device and mapping start sector. Changing the dmsetup command line for all users is probably not the best approach... Unless I am missing something... >> diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c >> index 27e2992ff249..1bf4ecda3012 100644 >> --- a/drivers/md/dm-target.c >> +++ b/drivers/md/dm-target.c >> @@ -118,6 +118,21 @@ EXPORT_SYMBOL(dm_unregister_target); >> */ >> static int io_err_ctr(struct dm_target *tt, unsigned int argc, char **args) >> { >> + struct dm_dev *ddev; >> + int ret; >> + >> + /* >> + * If we have an argument, assume it is the path to the target >> + * block device we are replacing. In this case, get the device >> + * so that we can copy its limits in io_err_io_hints(). >> + */ >> + if (argc) { >> + ret = dm_get_device(tt, args[0], dm_table_get_mode(tt->table), >> + &ddev); >> + if (ret == 0) >> + tt->private = ddev; > > No need to handle an error case here? Or, I guess it ignores an error for > compatibility when non-device argument is specified. Then, I'd like to add > a note here. Yes, exactly. Before the change, arguments were ignored. I will add a comment.
(Updating dm-devel address...) On Wed, Oct 25, 2023 at 02:53:17PM +0900, Damien Le Moal wrote: > On 10/25/23 14:22, Naohiro Aota wrote: > > On Tue, Oct 24, 2023 at 07:45:13AM +0900, Damien Le Moal wrote: > >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > >> index 37b48f63ae6a..5e4d887063d3 100644 > >> --- a/drivers/md/dm-table.c > >> +++ b/drivers/md/dm-table.c > >> @@ -1600,6 +1600,9 @@ static bool dm_table_supports_zoned_model(struct dm_table *t, > >> for (unsigned int i = 0; i < t->num_targets; i++) { > >> struct dm_target *ti = dm_table_get_target(t, i); > >> > >> + if (dm_target_is_wildcard(ti->type)) > >> + continue; > >> + > > > > This seems tricky to me. Currently, dm-error is the only dm target having > > DM_TARGET_WILDCARD. But, can we expect that will be so forever? > > Yes, I saw that. Not sure. Mike ? > > > Also, I considered what happens if the backing device is non-zoned > > one. dm_table_supports_zoned_model() returns true in that case. But, it is > > still reported as non-zoned as we copy non-zoned queue limits. So, it is > > OK ... but it is a bit tricky. > > Returning true for dm_table_supports_zoned_model() is not an issue. As the name > of the function says, this checks that the device table is OK with zoned device > but does not force the dm device to be zoned. I see. I just think it's confusing as it also checks the backing devices for other cases. > > Instead, how about implementing the .iterate_devices just like > > linear_iterate_devices? > > Because that would change how dm-error needs to be setup. Currently, there are > no arguments needed for the table entry for dm-error. If we define the > ->iterate_devices operation, we'll need a backing device and mapping start > sector. Changing the dmsetup command line for all users is probably not the best > approach... Unless I am missing something... I thought we could just return 0 when there is no backing device. But, I see, we need a start sector to call the callback function, which is pain. Then, how about continue the loop if (dm_target_supports_zoned_hm(ti->type) && !ti->type->iterate_devices) ? The target declares itself supporting zoned_hm and having NULL iterate_devices implies there is no backing device. Then, we can say that target supports the zoned model naturally.
On 10/25/23 15:21, Naohiro Aota wrote: > (Updating dm-devel address...) > > On Wed, Oct 25, 2023 at 02:53:17PM +0900, Damien Le Moal wrote: >> On 10/25/23 14:22, Naohiro Aota wrote: >>> On Tue, Oct 24, 2023 at 07:45:13AM +0900, Damien Le Moal wrote: >>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >>>> index 37b48f63ae6a..5e4d887063d3 100644 >>>> --- a/drivers/md/dm-table.c >>>> +++ b/drivers/md/dm-table.c >>>> @@ -1600,6 +1600,9 @@ static bool dm_table_supports_zoned_model(struct dm_table *t, >>>> for (unsigned int i = 0; i < t->num_targets; i++) { >>>> struct dm_target *ti = dm_table_get_target(t, i); >>>> >>>> + if (dm_target_is_wildcard(ti->type)) >>>> + continue; >>>> + >>> >>> This seems tricky to me. Currently, dm-error is the only dm target having >>> DM_TARGET_WILDCARD. But, can we expect that will be so forever? >> >> Yes, I saw that. Not sure. Mike ? >> >>> Also, I considered what happens if the backing device is non-zoned >>> one. dm_table_supports_zoned_model() returns true in that case. But, it is >>> still reported as non-zoned as we copy non-zoned queue limits. So, it is >>> OK ... but it is a bit tricky. >> >> Returning true for dm_table_supports_zoned_model() is not an issue. As the name >> of the function says, this checks that the device table is OK with zoned device >> but does not force the dm device to be zoned. > > I see. I just think it's confusing as it also checks the backing devices > for other cases. The checks are for target types and devices, to ensure that they are compatible. dm-error has no backing device, so the checks are reduced to checking the target type flags for zone support. >>> Instead, how about implementing the .iterate_devices just like >>> linear_iterate_devices? >> >> Because that would change how dm-error needs to be setup. Currently, there are >> no arguments needed for the table entry for dm-error. If we define the >> ->iterate_devices operation, we'll need a backing device and mapping start >> sector. Changing the dmsetup command line for all users is probably not the best >> approach... Unless I am missing something... > > I thought we could just return 0 when there is no backing device. But, I see, > we need a start sector to call the callback function, which is pain. > > Then, how about continue the loop if (dm_target_supports_zoned_hm(ti->type) > && !ti->type->iterate_devices) ? The target declares itself supporting > zoned_hm and having NULL iterate_devices implies there is no backing > device. Then, we can say that target supports the zoned model naturally. We could, but the point is that dm-error cannot be combined with other targets in a single table. If dm-error is used, it is everything, so there is no point in the iterate device loop. This is confusing as many examples out there seem to imply that dm-error can be used to emulate errors for a range of sectors only but that does not work. E.g., something like this: dmsetup create err_disk << EOF 0 8 linear $dev 0 8 1 error 9 $disk_size linear $dev 9 EOF does not work because dm-error does not have an iterate_devices method. This is why I implemented the zone support like I did instead of touching the iterate loops. But I would like to hear from Mike about this if there is a better way of doing things.
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 37b48f63ae6a..5e4d887063d3 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -1600,6 +1600,9 @@ static bool dm_table_supports_zoned_model(struct dm_table *t, for (unsigned int i = 0; i < t->num_targets; i++) { struct dm_target *ti = dm_table_get_target(t, i); + if (dm_target_is_wildcard(ti->type)) + continue; + if (dm_target_supports_zoned_hm(ti->type)) { if (!ti->type->iterate_devices || ti->type->iterate_devices(ti, device_not_zoned_model, diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c index 27e2992ff249..1bf4ecda3012 100644 --- a/drivers/md/dm-target.c +++ b/drivers/md/dm-target.c @@ -118,6 +118,21 @@ EXPORT_SYMBOL(dm_unregister_target); */ static int io_err_ctr(struct dm_target *tt, unsigned int argc, char **args) { + struct dm_dev *ddev; + int ret; + + /* + * If we have an argument, assume it is the path to the target + * block device we are replacing. In this case, get the device + * so that we can copy its limits in io_err_io_hints(). + */ + if (argc) { + ret = dm_get_device(tt, args[0], dm_table_get_mode(tt->table), + &ddev); + if (ret == 0) + tt->private = ddev; + } + /* * Return error for discards instead of -EOPNOTSUPP */ @@ -129,7 +144,10 @@ static int io_err_ctr(struct dm_target *tt, unsigned int argc, char **args) static void io_err_dtr(struct dm_target *tt) { - /* empty */ + struct dm_dev *ddev = tt->private; + + if (ddev) + dm_put_device(tt, ddev); } static int io_err_map(struct dm_target *tt, struct bio *bio) @@ -149,8 +167,27 @@ static void io_err_release_clone_rq(struct request *clone, { } +#ifdef CONFIG_BLK_DEV_ZONED +static int io_err_report_zones(struct dm_target *ti, + struct dm_report_zones_args *args, unsigned int nr_zones) +{ + return -EIO; +} +#else +#define io_err_report_zones NULL +#endif + static void io_err_io_hints(struct dm_target *ti, struct queue_limits *limits) { + struct dm_dev *ddev = ti->private; + + /* If we have a target device, copy its limits */ + if (ddev) { + struct request_queue *q = bdev_get_queue(ddev->bdev); + + memcpy(limits, &q->limits, sizeof(*limits)); + } + limits->max_discard_sectors = UINT_MAX; limits->max_hw_discard_sectors = UINT_MAX; limits->discard_granularity = 512; @@ -166,7 +203,7 @@ static long io_err_dax_direct_access(struct dm_target *ti, pgoff_t pgoff, static struct target_type error_target = { .name = "error", .version = {1, 6, 0}, - .features = DM_TARGET_WILDCARD, + .features = DM_TARGET_WILDCARD | DM_TARGET_ZONED_HM, .ctr = io_err_ctr, .dtr = io_err_dtr, .map = io_err_map, @@ -174,6 +211,7 @@ static struct target_type error_target = { .release_clone_rq = io_err_release_clone_rq, .io_hints = io_err_io_hints, .direct_access = io_err_dax_direct_access, + .report_zones = io_err_report_zones, }; int __init dm_target_init(void) diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c index eb9832b22b14..9b77ee05e8dd 100644 --- a/drivers/md/dm-zone.c +++ b/drivers/md/dm-zone.c @@ -297,6 +297,15 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q) WARN_ON_ONCE(queue_is_mq(q)); md->disk->nr_zones = bdev_nr_zones(md->disk->part0); + /* + * With dm-error (wildcard target), report zones will fail, so do + * nothing. dm-error will copy the zones limits itself. + */ + if (dm_table_get_wildcard_target(t)) { + md->nr_zones = md->disk->nr_zones; + return 0; + } + /* Check if zone append is natively supported */ if (dm_table_supports_zone_append(t)) { clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
dm-error is used in several test cases in the xfstests test suite to check the handling of IO errors in file syatems. However, with several file systems getting native support for zoned block devices (e.g. btrfs and f2fs), dm-error lack of zoned block device support creates problems as the file system attempt executing zone commands (e.g. a zone append operation) against a dm-error non-zoned block device, which causes various issues in the block layer (e.g. WARN_ON triggers). This patch adds supports for zoned block devices to dm-error, allowing an error table to be exposed as a zoned block device. This is done by relying on the first argument passed to dmsetup when creating the device table: if that first argument is a path to a backing block device, the dm-error device is created by copying the limits of the backing device, thus also copying its zone model. This is consistent with how xfstests creates dm-error devices (always passing the path to the backing device as the first argument). The zone support for dm-error requires the definition of the report_zones target type method, which is done by introducing the function io_err_report_zones(). Given that this function fails report zones operations (similarly to any other command issued to the dm-error device), dm_set_zones_restrictions() is tweaked to do nothing for a wildcard target to avoid failing zone revalidation. As the dm-error target does not implemt the iterate_devices method, dm_table_supports_zoned_model() is also changed to return true. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/md/dm-table.c | 3 +++ drivers/md/dm-target.c | 42 ++++++++++++++++++++++++++++++++++++++++-- drivers/md/dm-zone.c | 9 +++++++++ 3 files changed, 52 insertions(+), 2 deletions(-)