| Message ID | 20231025072332.465595-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | [v2] dm: error: Add support for zoned block devices | expand |
On Wed, Oct 25 2023 at 3:23P -0400, Damien Le Moal <dlemoal@kernel.org> wrote: > dm-error is used in several test cases in the xfstests test suite to > check the handling of IO errors in file systems. 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 implement the iterate_devices method, > dm_table_supports_zoned_model() is also changed to return true. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Tested-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > Changes from v1: > - Improved comment in io_err_ctr() about error from dm_get_device() > being ignored > - Fixed typos in the commit message > - Added review and tested-by tags Thanks for the improvements. But likely a v3 is worthwhile. Comment inlined below. > > drivers/md/dm-table.c | 3 +++ > drivers/md/dm-target.c | 45 ++++++++++++++++++++++++++++++++++++++++-- > drivers/md/dm-zone.c | 9 +++++++++ > 3 files changed, 55 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; > + > 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..fcb2ba2bffa2 100644 > --- a/drivers/md/dm-target.c > +++ b/drivers/md/dm-target.c > @@ -118,6 +118,24 @@ 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 backing > + * block device that we are replacing. In this case, get the device > + * so that we can copy its limits in io_err_io_hints(). If getting the > + * device fails (e.g. because the user did not specify a device file > + * path), ignore the error to be compatible with the normal use case > + * without any argument specified. > + */ > + 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 +147,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 +170,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; You don't need to explicitly copy the queue_limits, DM core (dm-table.c:dm_calculate_queue_limits) will stack up the underlying device's queue_limits as long as you implement io_err_iterate_devices (like dm-linear.c:linear_iterate_devices). With io_err_iterate_devices you shouldn't need to change io_err_io_hints -- but actually I could see there being a need to wrap setting the above no-device defaults only if (!ddev).. or: if (ddev) return; Mike
On 10/26/23 00:03, Mike Snitzer wrote: > On Wed, Oct 25 2023 at 3:23P -0400, > Damien Le Moal <dlemoal@kernel.org> wrote: > >> dm-error is used in several test cases in the xfstests test suite to >> check the handling of IO errors in file systems. 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 implement the iterate_devices method, >> dm_table_supports_zoned_model() is also changed to return true. >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Tested-by: Christoph Hellwig <hch@lst.de> >> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> Changes from v1: >> - Improved comment in io_err_ctr() about error from dm_get_device() >> being ignored >> - Fixed typos in the commit message >> - Added review and tested-by tags > > Thanks for the improvements. But likely a v3 is worthwhile. > > Comment inlined below. > >> >> drivers/md/dm-table.c | 3 +++ >> drivers/md/dm-target.c | 45 ++++++++++++++++++++++++++++++++++++++++-- >> drivers/md/dm-zone.c | 9 +++++++++ >> 3 files changed, 55 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; >> + >> 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..fcb2ba2bffa2 100644 >> --- a/drivers/md/dm-target.c >> +++ b/drivers/md/dm-target.c >> @@ -118,6 +118,24 @@ 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 backing >> + * block device that we are replacing. In this case, get the device >> + * so that we can copy its limits in io_err_io_hints(). If getting the >> + * device fails (e.g. because the user did not specify a device file >> + * path), ignore the error to be compatible with the normal use case >> + * without any argument specified. >> + */ >> + 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 +147,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 +170,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; > > > You don't need to explicitly copy the queue_limits, DM core > (dm-table.c:dm_calculate_queue_limits) will stack up the underlying > device's queue_limits as long as you implement io_err_iterate_devices > (like dm-linear.c:linear_iterate_devices). > > With io_err_iterate_devices you shouldn't need to change > io_err_io_hints -- but actually I could see there being a need to wrap > setting the above no-device defaults only if (!ddev).. or: > if (ddev) > return; Works for me, but I do not see how iterate_devices would work if there is no backing device specified (current default). Am I missing something ? Also, doesn't adding the iterate_devices method allow combining dm-error with other dm targets in the same table, something that is not possible currently ? So that would change dm-error usage. OK with me, but I want to make sure. But it may be that I am missing something because of the wildcard flag. Is do not fully understand how that one impacts anything. Is that flag responsible for preventing dm-error to be combined with other targets ? > > Mike >
On Wed, Oct 25 2023 at 5:34P -0400, Damien Le Moal <dlemoal@kernel.org> wrote: > On 10/26/23 00:03, Mike Snitzer wrote: > > On Wed, Oct 25 2023 at 3:23P -0400, > > Damien Le Moal <dlemoal@kernel.org> wrote: > > > >> dm-error is used in several test cases in the xfstests test suite to > >> check the handling of IO errors in file systems. 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 implement the iterate_devices method, > >> dm_table_supports_zoned_model() is also changed to return true. > >> > >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > >> Reviewed-by: Christoph Hellwig <hch@lst.de> > >> Tested-by: Christoph Hellwig <hch@lst.de> > >> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > >> --- > >> Changes from v1: > >> - Improved comment in io_err_ctr() about error from dm_get_device() > >> being ignored > >> - Fixed typos in the commit message > >> - Added review and tested-by tags > > > > Thanks for the improvements. But likely a v3 is worthwhile. > > > > Comment inlined below. > > > >> > >> drivers/md/dm-table.c | 3 +++ > >> drivers/md/dm-target.c | 45 ++++++++++++++++++++++++++++++++++++++++-- > >> drivers/md/dm-zone.c | 9 +++++++++ > >> 3 files changed, 55 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; > >> + > >> 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..fcb2ba2bffa2 100644 > >> --- a/drivers/md/dm-target.c > >> +++ b/drivers/md/dm-target.c > >> @@ -118,6 +118,24 @@ 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 backing > >> + * block device that we are replacing. In this case, get the device > >> + * so that we can copy its limits in io_err_io_hints(). If getting the > >> + * device fails (e.g. because the user did not specify a device file > >> + * path), ignore the error to be compatible with the normal use case > >> + * without any argument specified. > >> + */ > >> + 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 +147,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 +170,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; > > > > > > You don't need to explicitly copy the queue_limits, DM core > > (dm-table.c:dm_calculate_queue_limits) will stack up the underlying > > device's queue_limits as long as you implement io_err_iterate_devices > > (like dm-linear.c:linear_iterate_devices). > > > > With io_err_iterate_devices you shouldn't need to change > > io_err_io_hints -- but actually I could see there being a need to wrap > > setting the above no-device defaults only if (!ddev).. or: > > if (ddev) > > return; > > Works for me, but I do not see how iterate_devices would work if there is no > backing device specified (current default). Am I missing something ? You don't call 'fn' if the ddev (ti->private) is NULL. > Also, > doesn't adding the iterate_devices method allow combining dm-error with other dm > targets in the same table, something that is not possible currently ? So that > would change dm-error usage. OK with me, but I want to make sure. No, iterate_devices is purely for iterating within a given target's devices to call the provide 'fn'. What I suggested doesn't change combining dm-error with other targets in the same table. But we do allow that, think of a table with multiple linear targets and a subset that is the error target. > But it may be that I am missing something because of the wildcard flag. Is do > not fully understand how that one impacts anything. Is that flag responsible for > preventing dm-error to be combined with other targets ? /* * Indicates that a target may replace any target; even immutable targets. * .map, .map_rq, .clone_and_map_rq and .release_clone_rq are all defined. */ #define DM_TARGET_WILDCARD 0x00000008 Just means that the error target can replace all types of DM targets (its the only DM target that sets the DM_TARGET_WILDCARD flag). Mike
On 10/26/23 08:50, Mike Snitzer wrote: > On Wed, Oct 25 2023 at 5:34P -0400, > Damien Le Moal <dlemoal@kernel.org> wrote: > >> On 10/26/23 00:03, Mike Snitzer wrote: >>> On Wed, Oct 25 2023 at 3:23P -0400, >>> Damien Le Moal <dlemoal@kernel.org> wrote: >>> >>>> dm-error is used in several test cases in the xfstests test suite to >>>> check the handling of IO errors in file systems. 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 implement the iterate_devices method, >>>> dm_table_supports_zoned_model() is also changed to return true. >>>> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> >>>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>>> Tested-by: Christoph Hellwig <hch@lst.de> >>>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>>> --- >>>> Changes from v1: >>>> - Improved comment in io_err_ctr() about error from dm_get_device() >>>> being ignored >>>> - Fixed typos in the commit message >>>> - Added review and tested-by tags >>> >>> Thanks for the improvements. But likely a v3 is worthwhile. >>> >>> Comment inlined below. >>> >>>> >>>> drivers/md/dm-table.c | 3 +++ >>>> drivers/md/dm-target.c | 45 ++++++++++++++++++++++++++++++++++++++++-- >>>> drivers/md/dm-zone.c | 9 +++++++++ >>>> 3 files changed, 55 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; >>>> + >>>> 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..fcb2ba2bffa2 100644 >>>> --- a/drivers/md/dm-target.c >>>> +++ b/drivers/md/dm-target.c >>>> @@ -118,6 +118,24 @@ 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 backing >>>> + * block device that we are replacing. In this case, get the device >>>> + * so that we can copy its limits in io_err_io_hints(). If getting the >>>> + * device fails (e.g. because the user did not specify a device file >>>> + * path), ignore the error to be compatible with the normal use case >>>> + * without any argument specified. >>>> + */ >>>> + 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 +147,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 +170,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; >>> >>> >>> You don't need to explicitly copy the queue_limits, DM core >>> (dm-table.c:dm_calculate_queue_limits) will stack up the underlying >>> device's queue_limits as long as you implement io_err_iterate_devices >>> (like dm-linear.c:linear_iterate_devices). >>> >>> With io_err_iterate_devices you shouldn't need to change >>> io_err_io_hints -- but actually I could see there being a need to wrap >>> setting the above no-device defaults only if (!ddev).. or: >>> if (ddev) >>> return; >> >> Works for me, but I do not see how iterate_devices would work if there is no >> backing device specified (current default). Am I missing something ? > > You don't call 'fn' if the ddev (ti->private) is NULL. > >> Also, >> doesn't adding the iterate_devices method allow combining dm-error with other dm >> targets in the same table, something that is not possible currently ? So that >> would change dm-error usage. OK with me, but I want to make sure. > > No, iterate_devices is purely for iterating within a given target's > devices to call the provide 'fn'. > > What I suggested doesn't change combining dm-error with other targets > in the same table. But we do allow that, think of a table with > multiple linear targets and a subset that is the error target. I was confused about that because my test script had a problem and was failing to setup a dm-linear+dm-error combination. I now have that working and I can check the combination. So adding the iterate device now makes sense for the zone stuff. Will do that. Thanks for the details. Sending a v3. > >> But it may be that I am missing something because of the wildcard flag. Is do >> not fully understand how that one impacts anything. Is that flag responsible for >> preventing dm-error to be combined with other targets ? > > /* > * Indicates that a target may replace any target; even immutable targets. > * .map, .map_rq, .clone_and_map_rq and .release_clone_rq are all defined. > */ > #define DM_TARGET_WILDCARD 0x00000008 > > Just means that the error target can replace all types of DM targets > (its the only DM target that sets the DM_TARGET_WILDCARD flag). > > Mike >
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..fcb2ba2bffa2 100644 --- a/drivers/md/dm-target.c +++ b/drivers/md/dm-target.c @@ -118,6 +118,24 @@ 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 backing + * block device that we are replacing. In this case, get the device + * so that we can copy its limits in io_err_io_hints(). If getting the + * device fails (e.g. because the user did not specify a device file + * path), ignore the error to be compatible with the normal use case + * without any argument specified. + */ + 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 +147,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 +170,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 +206,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 +214,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);