| Message ID | 20240207021919.7989-1-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] scsi: target: Fix unmap setup during configuration | expand |
Hi Mike, On Tue, Feb 06, 2024 at 08:19:19PM -0600, Mike Christie wrote: > +static int target_try_configure_unmap(struct se_device *dev, > + const char *config_opt) > +{ > + if (!dev->transport->configure_unmap) > + return 0; > + > + if (!target_dev_configured(dev)) { > + pr_err("Generic Block Discard setup for %s requires device to be configured\n", > + config_opt); > + return -ENODEV; > + } > + > + if (!dev->transport->configure_unmap(dev)) { > + pr_err("Generic Block Discard setup for %s failed\n", > + config_opt); > + return -ENOSYS; > + } > + > + return 0; > +} > + > static ssize_t emulate_tpu_store(struct config_item *item, > const char *page, size_t count) > { > @@ -776,11 +797,9 @@ static ssize_t emulate_tpu_store(struct config_item *item, > * Discard supported is detected iblock_create_virtdevice(). > */ > if (flag && !da->max_unmap_block_desc_count) { > - if (!dev->transport->configure_unmap || You removed this check but that callout is not mandatory and exists just in 2 backstore modules. BR, Dmitry
On Wed, Feb 07, 2024 at 10:33:36AM +0300, Dmitry Bogdanov wrote: > Hi Mike, > > On Tue, Feb 06, 2024 at 08:19:19PM -0600, Mike Christie wrote: > > > +static int target_try_configure_unmap(struct se_device *dev, > > + const char *config_opt) > > +{ > > + if (!dev->transport->configure_unmap) > > + return 0; Oh, sorry, here it is :) > > + > > + if (!target_dev_configured(dev)) { > > + pr_err("Generic Block Discard setup for %s requires device to be configured\n", > > + config_opt); > > + return -ENODEV; > > + } > > + > > + if (!dev->transport->configure_unmap(dev)) { > > + pr_err("Generic Block Discard setup for %s failed\n", > > + config_opt); > > + return -ENOSYS; > > + } > > + > > + return 0; > > +} > > + > > static ssize_t emulate_tpu_store(struct config_item *item, > > const char *page, size_t count) > > { > > @@ -776,11 +797,9 @@ static ssize_t emulate_tpu_store(struct config_item *item, > > * Discard supported is detected iblock_create_virtdevice(). > > */ > > if (flag && !da->max_unmap_block_desc_count) { > > - if (!dev->transport->configure_unmap || > You removed this check but that callout is not mandatory and exists just in > 2 backstore modules. >
st 7. 2. 2024 v 3:19 odesÃlatel Mike Christie <michael.christie@oracle.com> napsal: > > +static int target_try_configure_unmap(struct se_device *dev, > + const char *config_opt) > +{ > + if (!dev->transport->configure_unmap) > + return 0; With this patch, if the configure_unmap callback is NULL then we return 0, implying that discard is supported. Before, a NULL configure_unmap callback triggered an error: if (flag && !da->max_unmap_block_desc_count) { if (!dev->transport->configure_unmap || <<------ !dev->transport->configure_unmap(dev)) { pr_err("Generic Block Discard not supported\n"); return -ENOSYS; } } Shouldn't you return -ENOSYS in target_try_configure_unmap() if configure_unmap is NULL? Maurizio
On 2/7/24 4:40 AM, Maurizio Lombardi wrote: > st 7. 2. 2024 v 3:19 odesÃlatel Mike Christie > <michael.christie@oracle.com> napsal: >> >> +static int target_try_configure_unmap(struct se_device *dev, >> + const char *config_opt) >> +{ >> + if (!dev->transport->configure_unmap) >> + return 0; > > With this patch, if the configure_unmap callback is NULL then we > return 0, implying that discard is supported. > > Before, a NULL configure_unmap callback triggered an error: > > if (flag && !da->max_unmap_block_desc_count) { > if (!dev->transport->configure_unmap || <<------ > !dev->transport->configure_unmap(dev)) { > pr_err("Generic Block Discard not supported\n"); > return -ENOSYS; > } > } > > Shouldn't you return -ENOSYS in target_try_configure_unmap() if > configure_unmap is NULL? > You are right. Will fix.
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c index a5f58988130a..d87f6eba4eda 100644 --- a/drivers/target/target_core_configfs.c +++ b/drivers/target/target_core_configfs.c @@ -759,6 +759,27 @@ static ssize_t emulate_tas_store(struct config_item *item, return count; } +static int target_try_configure_unmap(struct se_device *dev, + const char *config_opt) +{ + if (!dev->transport->configure_unmap) + return 0; + + if (!target_dev_configured(dev)) { + pr_err("Generic Block Discard setup for %s requires device to be configured\n", + config_opt); + return -ENODEV; + } + + if (!dev->transport->configure_unmap(dev)) { + pr_err("Generic Block Discard setup for %s failed\n", + config_opt); + return -ENOSYS; + } + + return 0; +} + static ssize_t emulate_tpu_store(struct config_item *item, const char *page, size_t count) { @@ -776,11 +797,9 @@ static ssize_t emulate_tpu_store(struct config_item *item, * Discard supported is detected iblock_create_virtdevice(). */ if (flag && !da->max_unmap_block_desc_count) { - if (!dev->transport->configure_unmap || - !dev->transport->configure_unmap(dev)) { - pr_err("Generic Block Discard not supported\n"); - return -ENOSYS; - } + ret = target_try_configure_unmap(dev, "emulate_tpu"); + if (ret) + return ret; } da->emulate_tpu = flag; @@ -806,11 +825,9 @@ static ssize_t emulate_tpws_store(struct config_item *item, * Discard supported is detected iblock_create_virtdevice(). */ if (flag && !da->max_unmap_block_desc_count) { - if (!dev->transport->configure_unmap || - !dev->transport->configure_unmap(dev)) { - pr_err("Generic Block Discard not supported\n"); - return -ENOSYS; - } + ret = target_try_configure_unmap(dev, "emulate_tpws"); + if (ret) + return ret; } da->emulate_tpws = flag; @@ -1022,12 +1039,9 @@ static ssize_t unmap_zeroes_data_store(struct config_item *item, * Discard supported is detected iblock_configure_device(). */ if (flag && !da->max_unmap_block_desc_count) { - if (!dev->transport->configure_unmap || - !dev->transport->configure_unmap(dev)) { - pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set because max_unmap_block_desc_count is zero\n", - da->da_dev); - return -ENOSYS; - } + ret = target_try_configure_unmap(dev, "unmap_zeroes_data"); + if (ret) + return ret; } da->unmap_zeroes_data = flag; pr_debug("dev[%p]: SE Device Thin Provisioning LBPRZ bit: %d\n",
This issue was found and also debugged by Carl Lei <me@xecycle.info>. If the device is not enabled, iblock/file will have not setup their se_device to bdev/file mappings. If a user tries to config the unmap settings at this time, we will then crash trying to access a NULL pointer where the bdev/file should be. This patch adds a check to make sure the device is configured before we try to call the configure_unmap callout. Fixes: 34bd1dcacf0d ("scsi: target: Detect UNMAP support post configuration") Reported-by: Carl Lei <me@xecycle.info> Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/target/target_core_configfs.c | 46 +++++++++++++++++---------- 1 file changed, 30 insertions(+), 16 deletions(-)