| Message ID | 20240821213048.726082-1-priv.luk@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | dm: Implement set_read_only | expand |
On Wed, 21 Aug 2024, Łukasz Patron wrote: > This lets us change the read-only flag for device mapper block devices > via the BLKROSET ioctl. > > Signed-off-by: Łukasz Patron <priv.luk@gmail.com> > --- > drivers/md/dm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 87bb90303435..538a93e596d7 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo) > return dm_get_geometry(md, geo); > } > > +static int dm_blk_set_read_only(struct block_device *bdev, bool ro) > +{ > + set_disk_ro(bdev->bd_disk, ro); > + return 0; > +} > + > static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx, > struct block_device **bdev) > { > @@ -3666,6 +3672,7 @@ static const struct block_device_operations dm_blk_dops = { > .release = dm_blk_close, > .ioctl = dm_blk_ioctl, > .getgeo = dm_blk_getgeo, > + .set_read_only = dm_blk_set_read_only, > .report_zones = dm_blk_report_zones, > .pr_ops = &dm_pr_ops, > .owner = THIS_MODULE > -- > 2.46.0 Hi Device mapper already calls set_disk_ro in the do_resume function. So, the problem here is that the value set using set_read_only will be overwritten as soon as a new table will be loaded. 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? 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. Mikulas
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. >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. --- 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; } On Tue, Aug 27, 2024 at 7:52 PM Mikulas Patocka <mpatocka@redhat.com> wrote: On Wed, 21 Aug 2024, Łukasz Patron wrote: > This lets us change the read-only flag for device mapper block devices > via the BLKROSET ioctl. > > Signed-off-by: Łukasz Patron <priv.luk@gmail.com> > --- > drivers/md/dm.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 87bb90303435..538a93e596d7 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo) > return dm_get_geometry(md, geo); > } > > +static int dm_blk_set_read_only(struct block_device *bdev, bool ro) > +{ > + set_disk_ro(bdev->bd_disk, ro); > + return 0; > +} > + > static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx, > struct block_device **bdev) > { > @@ -3666,6 +3672,7 @@ static const struct block_device_operations dm_blk_dops = { > .release = dm_blk_close, > .ioctl = dm_blk_ioctl, > .getgeo = dm_blk_getgeo, > + .set_read_only = dm_blk_set_read_only, > .report_zones = dm_blk_report_zones, > .pr_ops = &dm_pr_ops, > .owner = THIS_MODULE > -- > 2.46.0 Hi Device mapper already calls set_disk_ro in the do_resume function. So, the problem here is that the value set using set_read_only will be overwritten as soon as a new table will be loaded. 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? 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. Mikulas
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; > }
diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 87bb90303435..538a93e596d7 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo) return dm_get_geometry(md, geo); } +static int dm_blk_set_read_only(struct block_device *bdev, bool ro) +{ + set_disk_ro(bdev->bd_disk, ro); + return 0; +} + static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx, struct block_device **bdev) { @@ -3666,6 +3672,7 @@ static const struct block_device_operations dm_blk_dops = { .release = dm_blk_close, .ioctl = dm_blk_ioctl, .getgeo = dm_blk_getgeo, + .set_read_only = dm_blk_set_read_only, .report_zones = dm_blk_report_zones, .pr_ops = &dm_pr_ops, .owner = THIS_MODULE
This lets us change the read-only flag for device mapper block devices via the BLKROSET ioctl. Signed-off-by: Łukasz Patron <priv.luk@gmail.com> --- drivers/md/dm.c | 7 +++++++ 1 file changed, 7 insertions(+)