* [PATCH] nvme: Fix zns drives without append support to export correct permissions [not found] <CGME20220311201638eucas1p18b9c5ecb3442b5a53bba233591d34db0@eucas1p1.samsung.com> @ 2022-03-11 20:16 ` Pankaj Raghav 2022-03-11 20:49 ` Adam Manzanares ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Pankaj Raghav @ 2022-03-11 20:16 UTC (permalink / raw) To: Javier González, Keith Busch, Christoph Hellwig, Sagi Grimberg, Damien Le Moal, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe Cc: Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme, Pankaj Raghav This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone Append support read-only") exported zoned namespaces without append support to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the ns->flags in nvme_update_zone_info and later nvme_update_disk_info will check for this flag and set the disk as ro. But later this commit 73d90386b559 ("nvme: cleanup zone information initialization") rearranged nvme_update_disk_info to be called before nvme_update_zone_info thereby not marking the disk as ro. The call order cannot be just reverted because nvme_update_zone_info sets certain queue parameters such as zone_write_granularity that depend on the prior call to nvme_update_disk_info. Call nvme_update_disk_info after nvme_update_zone_info again so that the permission for ZNS drives are marked correctly. Fixes: 73d90386b559 ("nvme: cleanup zone information initialization") Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> --- drivers/nvme/host/core.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 51c08f206cbf..67a78653b07c 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id) ret = nvme_update_zone_info(ns, lbaf); if (ret) goto out_unfreeze; + /* nvme_update_zone_info might set the namespace to be marked + * as read-only. Call nvme_update_disk_info so that the disk + * is updated with the appropriate permission. + */ + nvme_update_disk_info(ns->disk, ns, id); } set_bit(NVME_NS_READY, &ns->flags); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-11 20:16 ` [PATCH] nvme: Fix zns drives without append support to export correct permissions Pankaj Raghav @ 2022-03-11 20:49 ` Adam Manzanares 2022-03-12 8:14 ` Damien Le Moal 2022-03-13 13:15 ` Sagi Grimberg 2 siblings, 0 replies; 9+ messages in thread From: Adam Manzanares @ 2022-03-11 20:49 UTC (permalink / raw) To: Pankaj Raghav Cc: Javier González, Keith Busch, Christoph Hellwig, Sagi Grimberg, Damien Le Moal, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe, Luis Chamberlain, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme@lists.infradead.org On Fri, Mar 11, 2022 at 09:16:08PM +0100, Pankaj Raghav wrote: > This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone > Append support read-only") exported zoned namespaces without append support > to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the > ns->flags in nvme_update_zone_info and later nvme_update_disk_info will > check for this flag and set the disk as ro. > > But later this commit 73d90386b559 ("nvme: cleanup zone information > initialization") rearranged nvme_update_disk_info to be called before > nvme_update_zone_info thereby not marking the disk as ro. The call order > cannot be just reverted because nvme_update_zone_info sets certain queue > parameters such as zone_write_granularity that depend on the prior call > to nvme_update_disk_info. > > Call nvme_update_disk_info after nvme_update_zone_info again so that the > permission for ZNS drives are marked correctly. > > Fixes: 73d90386b559 ("nvme: cleanup zone information initialization") > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/nvme/host/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 51c08f206cbf..67a78653b07c 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id) > ret = nvme_update_zone_info(ns, lbaf); > if (ret) > goto out_unfreeze; > + /* nvme_update_zone_info might set the namespace to be marked > + * as read-only. Call nvme_update_disk_info so that the disk > + * is updated with the appropriate permission. > + */ > + nvme_update_disk_info(ns->disk, ns, id); Shouldn't we make this conditional? > } > > set_bit(NVME_NS_READY, &ns->flags); > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-11 20:16 ` [PATCH] nvme: Fix zns drives without append support to export correct permissions Pankaj Raghav 2022-03-11 20:49 ` Adam Manzanares @ 2022-03-12 8:14 ` Damien Le Moal 2022-03-14 8:36 ` Pankaj Raghav 2022-03-13 13:15 ` Sagi Grimberg 2 siblings, 1 reply; 9+ messages in thread From: Damien Le Moal @ 2022-03-12 8:14 UTC (permalink / raw) To: Pankaj Raghav, Javier González, Keith Busch, Christoph Hellwig, Sagi Grimberg, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe Cc: Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On 3/12/22 05:16, Pankaj Raghav wrote: > This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone > Append support read-only") exported zoned namespaces without append support > to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the > ns->flags in nvme_update_zone_info and later nvme_update_disk_info will > check for this flag and set the disk as ro. > > But later this commit 73d90386b559 ("nvme: cleanup zone information > initialization") rearranged nvme_update_disk_info to be called before > nvme_update_zone_info thereby not marking the disk as ro. The call order > cannot be just reverted because nvme_update_zone_info sets certain queue > parameters such as zone_write_granularity that depend on the prior call > to nvme_update_disk_info. > > Call nvme_update_disk_info after nvme_update_zone_info again so that the > permission for ZNS drives are marked correctly. > > Fixes: 73d90386b559 ("nvme: cleanup zone information initialization") > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/nvme/host/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 51c08f206cbf..67a78653b07c 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id) > ret = nvme_update_zone_info(ns, lbaf); > if (ret) > goto out_unfreeze; > + /* nvme_update_zone_info might set the namespace to be marked > + * as read-only. Call nvme_update_disk_info so that the disk > + * is updated with the appropriate permission. > + */ Multi-line comment block should start with a "/*" line without text. > + nvme_update_disk_info(ns->disk, ns, id); It may make more sense to move the "set_disk_ro()" call at the end of nvme_update_zone_info() into a different helper (say nvme_set_disk_mode()) and call that new helper here to avoid going through again all the settings that nvme_update_disk_info() does. > } > > set_bit(NVME_NS_READY, &ns->flags); -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-12 8:14 ` Damien Le Moal @ 2022-03-14 8:36 ` Pankaj Raghav 2022-03-14 8:44 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Pankaj Raghav @ 2022-03-14 8:36 UTC (permalink / raw) To: Damien Le Moal, Javier González, Keith Busch, Christoph Hellwig, Sagi Grimberg, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe Cc: Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme Hi Damien, On 2022-03-12 09:14, Damien Le Moal wrote: > On 3/12/22 05:16, Pankaj Raghav wrote: >> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization") >> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> >> --- >> drivers/nvme/host/core.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 51c08f206cbf..67a78653b07c 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id) >> ret = nvme_update_zone_info(ns, lbaf); >> if (ret) >> goto out_unfreeze; >> + /* nvme_update_zone_info might set the namespace to be marked >> + * as read-only. Call nvme_update_disk_info so that the disk >> + * is updated with the appropriate permission. >> + */ > > Multi-line comment block should start with a "/*" line without text. > >> + nvme_update_disk_info(ns->disk, ns, id); > > It may make more sense to move the "set_disk_ro()" call at the end of > nvme_update_zone_info() into a different helper (say > nvme_set_disk_mode()) and call that new helper here to avoid going > through again all the settings that nvme_update_disk_info() does. That is a good idea. Based on a quick check, we don't do NVME_NS_FORCE_NO anywhere except zns.c, so would it make sense to do the following change then? diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 51c08f206cbf..cde33f2a3a5a 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1855,8 +1855,7 @@ static void nvme_update_disk_info(struct gendisk *disk, blk_queue_max_write_zeroes_sectors(disk->queue, ns->ctrl->max_zeroes_sectors); - set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO) || - test_bit(NVME_NS_FORCE_RO, &ns->flags)); + set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO)); } static inline bool nvme_first_scan(struct gendisk *disk) diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index e7ccdb119ede..b6800bdd6ea9 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -607,6 +607,11 @@ static inline bool nvme_is_path_error(u16 status) return (status & 0x700) == 0x300; } +static inline void nvme_set_disk_mode_ro(struct nvme_ns *ns) +{ + set_disk_ro(ns->disk, test_bit(NVME_NS_FORCE_RO, &ns->flags)); +} + /* * Fill in the status and result information from the CQE, and then figure out * if blk-mq will need to use IPI magic to complete the request, and if yes do diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c index 9f81beb4df4e..4ab685fa02b4 100644 --- a/drivers/nvme/host/zns.c +++ b/drivers/nvme/host/zns.c @@ -113,6 +113,7 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf) blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q); blk_queue_max_open_zones(q, le32_to_cpu(id->mor) + 1); blk_queue_max_active_zones(q, le32_to_cpu(id->mar) + 1); + nvme_set_disk_mode_ro(ns); free_data: kfree(id); return status; -- Regards, Pankaj ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-14 8:36 ` Pankaj Raghav @ 2022-03-14 8:44 ` Christoph Hellwig 2022-03-14 9:21 ` Damien Le Moal 2022-03-14 10:19 ` Pankaj Raghav 0 siblings, 2 replies; 9+ messages in thread From: Christoph Hellwig @ 2022-03-14 8:44 UTC (permalink / raw) To: Pankaj Raghav Cc: Damien Le Moal, Javier González, Keith Busch, Christoph Hellwig, Sagi Grimberg, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe, Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote: > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 51c08f206cbf..cde33f2a3a5a 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1855,8 +1855,7 @@ static void nvme_update_disk_info(struct gendisk > *disk, > blk_queue_max_write_zeroes_sectors(disk->queue, > ns->ctrl->max_zeroes_sectors); > > - set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO) || > - test_bit(NVME_NS_FORCE_RO, &ns->flags)); > + set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO)); > } This creates a race window during revalidation where the zone will be set writable here only to be set read-only again a little later. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-14 8:44 ` Christoph Hellwig @ 2022-03-14 9:21 ` Damien Le Moal 2022-03-14 10:19 ` Pankaj Raghav 1 sibling, 0 replies; 9+ messages in thread From: Damien Le Moal @ 2022-03-14 9:21 UTC (permalink / raw) To: Christoph Hellwig, Pankaj Raghav Cc: Javier González, Keith Busch, Sagi Grimberg, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe, Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On 3/14/22 17:44, Christoph Hellwig wrote: > On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote: >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 51c08f206cbf..cde33f2a3a5a 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1855,8 +1855,7 @@ static void nvme_update_disk_info(struct gendisk >> *disk, >> blk_queue_max_write_zeroes_sectors(disk->queue, >> ns->ctrl->max_zeroes_sectors); >> >> - set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO) || >> - test_bit(NVME_NS_FORCE_RO, &ns->flags)); >> + set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO)); >> } > > This creates a race window during revalidation where the zone will be > set writable here only to be set read-only again a little later. Then the only good solution is to have the RO flag set before calling device_add_disk(), no ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-14 8:44 ` Christoph Hellwig 2022-03-14 9:21 ` Damien Le Moal @ 2022-03-14 10:19 ` Pankaj Raghav 1 sibling, 0 replies; 9+ messages in thread From: Pankaj Raghav @ 2022-03-14 10:19 UTC (permalink / raw) To: Christoph Hellwig Cc: Damien Le Moal, Javier González, Keith Busch, Sagi Grimberg, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe, Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On 2022-03-14 09:44, Christoph Hellwig wrote: > On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote: >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 51c08f206cbf..cde33f2a3a5a 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1855,8 +1855,7 @@ static void nvme_update_disk_info(struct gendisk >> *disk, >> blk_queue_max_write_zeroes_sectors(disk->queue, >> ns->ctrl->max_zeroes_sectors); >> >> - set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO) || >> - test_bit(NVME_NS_FORCE_RO, &ns->flags)); >> + set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO)); >> } > > This creates a race window during revalidation where the zone will be > set writable here only to be set read-only again a little later. We call the blk_revalidate_zone after nvme_update_zone_info. So before we revalidate, the correct permission is already set. // sets the disk to ro based on nsattr nvme_update_disk_info(ns->disk, ns, id); if (ns->head->ids.csi == NVME_CSI_ZNS) { // Sets the disk to ro based on append ret = nvme_update_zone_info(ns, lbaf); if (ret) goto out_unfreeze; } set_bit(NVME_NS_READY, &ns->flags); blk_mq_unfreeze_queue(ns->disk->queue); if (blk_queue_is_zoned(ns->queue)) { // Revalidate is called after the permission is set ret = nvme_revalidate_zones(ns); if (ret && !nvme_first_scan(ns->disk)) return ret; } Am I missing something? -- Regards, Pankaj ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-11 20:16 ` [PATCH] nvme: Fix zns drives without append support to export correct permissions Pankaj Raghav 2022-03-11 20:49 ` Adam Manzanares 2022-03-12 8:14 ` Damien Le Moal @ 2022-03-13 13:15 ` Sagi Grimberg 2022-03-14 6:21 ` Christoph Hellwig 2 siblings, 1 reply; 9+ messages in thread From: Sagi Grimberg @ 2022-03-13 13:15 UTC (permalink / raw) To: Pankaj Raghav, Javier González, Keith Busch, Christoph Hellwig, Damien Le Moal, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe Cc: Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On 3/11/22 22:16, Pankaj Raghav wrote: > This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone > Append support read-only") exported zoned namespaces without append support > to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the > ns->flags in nvme_update_zone_info and later nvme_update_disk_info will > check for this flag and set the disk as ro. > > But later this commit 73d90386b559 ("nvme: cleanup zone information > initialization") rearranged nvme_update_disk_info to be called before > nvme_update_zone_info thereby not marking the disk as ro. The call order > cannot be just reverted because nvme_update_zone_info sets certain queue > parameters such as zone_write_granularity that depend on the prior call > to nvme_update_disk_info. > > Call nvme_update_disk_info after nvme_update_zone_info again so that the > permission for ZNS drives are marked correctly. > > Fixes: 73d90386b559 ("nvme: cleanup zone information initialization") > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > drivers/nvme/host/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 51c08f206cbf..67a78653b07c 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id) > ret = nvme_update_zone_info(ns, lbaf); > if (ret) > goto out_unfreeze; > + /* nvme_update_zone_info might set the namespace to be marked > + * as read-only. Call nvme_update_disk_info so that the disk > + * is updated with the appropriate permission. > + */ > + nvme_update_disk_info(ns->disk, ns, id); There is a call just above this, why add another call? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nvme: Fix zns drives without append support to export correct permissions 2022-03-13 13:15 ` Sagi Grimberg @ 2022-03-14 6:21 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2022-03-14 6:21 UTC (permalink / raw) To: Sagi Grimberg Cc: Pankaj Raghav, Javier González, Keith Busch, Christoph Hellwig, Damien Le Moal, Johannes Thumshirn, Chaitanya Kulkarni, Jens Axboe, Luis Chamberlain, Adam Manzanares, kanchan Joshi, Pankaj Raghav, Kanchan Joshi, linux-nvme On Sun, Mar 13, 2022 at 03:15:08PM +0200, Sagi Grimberg wrote: > There is a call just above this, why add another call? NVME_NS_FORCE_RO is set in nvme_update_zone_info, which is called after nvme_update_disk_info. I think the suggestion from Damien to move the set_disk_ro call out of nvme_update_disk_info and directly into nvme_update_ns_info so that it can be done later is the right thing to do here. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-14 10:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20220311201638eucas1p18b9c5ecb3442b5a53bba233591d34db0@eucas1p1.samsung.com>
2022-03-11 20:16 ` [PATCH] nvme: Fix zns drives without append support to export correct permissions Pankaj Raghav
2022-03-11 20:49 ` Adam Manzanares
2022-03-12 8:14 ` Damien Le Moal
2022-03-14 8:36 ` Pankaj Raghav
2022-03-14 8:44 ` Christoph Hellwig
2022-03-14 9:21 ` Damien Le Moal
2022-03-14 10:19 ` Pankaj Raghav
2022-03-13 13:15 ` Sagi Grimberg
2022-03-14 6:21 ` Christoph Hellwig
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.