Re: [PATCH] dm: Implement set_read_only

[Mikulas Patocka <mpatocka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]



On Wed, 28 Aug 2024, Łukasz Patron wrote:

> Hi
> 
> >I'd like to ask why is this patch needed? Why do you want to set read-only
> >status using this ioctl instead of using the existing table flag?
> 
> I basically just wanted to be able to use `blockdev --setrw {}` on
> Android for a block device that had its table mapped as read only. I
> believe that used to work on 5.10 or so, but not anymore.

Yes, I looked at the older kernel and it will just flip the read-only flag 
regardress of whether the driver supports it or not.

What specific partition do you need to write to? Is it possible to just 
reload the table instead of using blockdev --setrw?

Is it required for rooting the phone or for some other activity?

> >If this is needed, we need to add another flag that is being set by
> >dm_blk_set_read_only, so that dm_blk_set_read_only and dm_resume won't
> >step over each other's changes.
> 
> The following diff should address that, however I noticed that
> set_disk_ro() itself, triggers an uevent message that makes upstream
> lvm2/udev/10-dm.rules.in <http://10-dm.rules.in> disable a dm device, so not
> sure if this is
> good to have, after all.

This patch doesn't address that - when someone loads a new table and then 
does suspend+resume to swap the table, set_disk_ro will be called and the 
value specified by dm_blk_set_read_only will be overwritten.

Another problem is that if the table is read-only and you forcefully flip 
it to read-write, then the underlying devices will still be read-only and 
you would be writing to them. This is something that shouldn't be done. 
Unforunatelly, older lvm does that - so the kernel just prints a warning 
instead of rejecting the write. But I just don't want to add more places 
where we are writing to read-only devices.

Mikulas

> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -412,6 +412,19 @@ static int dm_blk_getgeo(struct block_device *bdev,
> struct hd_geometry *geo)
>   static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
>  {
> +	struct mapped_device *md = bdev->bd_disk->private_data;
> +	int srcu_idx;
> +	struct dm_table *table;
> +
> +	table = dm_get_live_table(md, &srcu_idx);
> +	if (table) {
> +		if (ro)
> +			table->mode &= ~BLK_OPEN_WRITE;
> +		else
> +			table->mode = ~BLK_OPEN_WRITE;
> +	}
> +	dm_put_live_table(md, srcu_idx);
> +
>  	set_disk_ro(bdev->bd_disk, ro);
>  	return 0;
>  }