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 <mpato...@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....@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


Reply via email to