[PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Sagi Grimberg]

The offending commit breaks BLKROSET ioctl because a device
revalidation will blindly override BLKROSET setting.

The author should resubmit this commit in a way that does
not break BLKROSET.

Fixes: 1293477f4f32 ("nvme: set gendisk read only based on nsattr")
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 40ca71b29bb9..f5365672ca82 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2057,11 +2057,6 @@ static void nvme_update_disk_info(struct gendisk *disk,
 
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
-
-	if (id->nsattr & NVME_NS_ATTR_RO)
-		set_disk_ro(disk, true);
-	else
-		set_disk_ro(disk, false);
 }
 
 static inline bool nvme_first_scan(struct gendisk *disk)
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Christoph Hellwig]

On Fri, Nov 06, 2020 at 09:18:35AM -0800, Sagi Grimberg wrote:
> The offending commit breaks BLKROSET ioctl because a device
> revalidation will blindly override BLKROSET setting.
> 
> The author should resubmit this commit in a way that does
> not break BLKROSET.

Let's fix this properly instead of a blind revert.

> -	if (id->nsattr & NVME_NS_ATTR_RO)
> -		set_disk_ro(disk, true);
> -	else
> -		set_disk_ro(disk, false);

Just dropping the else part is probably enough for now, although
as a follow on we should probably add a "hard read-only" state to
the block layer.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Sagi Grimberg]

>> The offending commit breaks BLKROSET ioctl because a device
>> revalidation will blindly override BLKROSET setting.
>>
>> The author should resubmit this commit in a way that does
>> not break BLKROSET.
> 
> Let's fix this properly instead of a blind revert.

I've asked Chaitanya to address this, but I guess he didn't
get to it so at the very least lets not keep the breakage.

>> -	if (id->nsattr & NVME_NS_ATTR_RO)
>> -		set_disk_ro(disk, true);
>> -	else
>> -		set_disk_ro(disk, false);
> 
> Just dropping the else part is probably enough for now, although
> as a follow on we should probably add a "hard read-only" state to
> the block layer.

Removing the else would have an incorrect behavior. I think its better
to not support it (until we support it correctly) than to support it
incorrectly.

Process-wise, I think we should revert this because it was added
incorrectly. Having a "hard-ro" in the block layer is unlikely to be
backported to stable. Hence I think we should revert it and re-add it
correctly this time.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Sagi Grimberg]

>>>> The offending commit breaks BLKROSET ioctl because a device
>>>> revalidation will blindly override BLKROSET setting.
>>>>
>>>> The author should resubmit this commit in a way that does
>>>> not break BLKROSET.
>>> Let's fix this properly instead of a blind revert.
>>
>> I've asked Chaitanya to address this, but I guess he didn't
>> get to it so at the very least lets not keep the breakage.
> 
> Yes it is on the list. I am really sorry I got busy with blocktrace [1] 
> work for last couple of weeks.

Not a problem at all really, we are all busy.

> Sagi, I am okay if you want to fix this. Otherwise I'll try and get this 
> and the target side fix next week for sure.

There is no target side fix here, it's just the host that needs fixing.

Still I think this needs to be reverted first and pulled into stable and
re-added correctly. I'm waiting to hear Christoph's opinion here.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Christoph Hellwig]

On Fri, Nov 06, 2020 at 11:02:28AM -0800, Sagi Grimberg wrote:
>> Just dropping the else part is probably enough for now, although
>> as a follow on we should probably add a "hard read-only" state to
>> the block layer.
>
> Removing the else would have an incorrect behavior. I think its better
> to not support it (until we support it correctly) than to support it
> incorrectly.

I strongly disagree.  When we see a write protected namespaces doing a
set_disk_ro is the right thing to do to prevent folks from writing to
it and getting bogged up in tons of warnings.   I think that use case
is much more important than the corner case of clearing it later.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: revert commit 1293477f4f32 as it is breaking BLKROSET

[Sagi Grimberg]

>>> Just dropping the else part is probably enough for now, although
>>> as a follow on we should probably add a "hard read-only" state to
>>> the block layer.
>>
>> Removing the else would have an incorrect behavior. I think its better
>> to not support it (until we support it correctly) than to support it
>> incorrectly.
> 
> I strongly disagree.  When we see a write protected namespaces doing a
> set_disk_ro is the right thing to do to prevent folks from writing to
> it and getting bogged up in tons of warnings.   I think that use case
> is much more important than the corner case of clearing it later.

OK, I can send a v2 removing the clear.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme