Re: [PATCH v2 31/34] block: use file->f_op to indicate restricted writes

Christian Brauner
2024-02-02T11:45:53Z

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

On Thu, Feb 01, 2024 at 06:36:31PM +0100, Jan Kara wrote:
> On Thu 01-02-24 17:16:02, Christian Brauner wrote:
> > On Thu, Feb 01, 2024 at 12:08:58PM +0100, Jan Kara wrote:
> > > On Tue 23-01-24 14:26:48, Christian Brauner wrote:
> > > > Make it possible to detected a block device that was opened with
> > > > restricted write access solely based on its file operations that it was
> > > > opened with. This avoids wasting an FMODE_* flag.
> > > >
> > > > def_blk_fops isn't needed to check whether something is a block device
> > > > checking the inode type is enough for that. And def_blk_fops_restricted
> > > > can be kept private to the block layer.
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > >
> > > I don't think we need def_blk_fops_restricted. If we have BLK_OPEN_WRITE
> > > file against a bdev with bdev_writes_blocked() == true, we are sure this is
> > > the handle blocking other writes so we can unblock them in
> > > bdev_yield_write_access()...
>
> ...
>
> > - if (mode & BLK_OPEN_RESTRICT_WRITES)
> > + if (mode & BLK_OPEN_WRITE) {
> > + if (bdev_writes_blocked(bdev))
> > + bdev_unblock_writes(bdev);
> > + else
> > + bdev->bd_writers--;
> > + }
> > + if (bdev_file->f_op == &def_blk_fops_restricted)
>
> Uh, why are you leaving def_blk_fops_restricted check here? I'd expect you
> can delete def_blk_fops_restricted completely...

Copy-paste error when dumping this into here. Here's the full patch.

[-- Attachment #2: 0001-block-don-t-rely-on-BLK_OPEN_RESTRICT_WRITES-when-yi.patch --]
[-- Type: text/x-diff, Size: 1910 bytes --]

From 25609e947674d2c24fb18edd0dabb5a49f27f23d Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 23 Jan 2024 14:26:48 +0100
Subject: [PATCH] block: don't rely on BLK_OPEN_RESTRICT_WRITES when yielding
write access

Make it possible to detected a block device that was opened with
restricted write access based only on BLK_OPEN_WRITE and
bdev->bd_writers < 0 so we won't have to claim another FMODE_* flag.

Link: https://lore.kernel.org/r/20240123-vfs-bdev-file-v2-31-adbd023e19cc@kernel.org
base-commit: 0bd1bf95a554f5f877724c27dbe33d4db0af4d0c
change-id: 20240129-vfs-bdev-file-bd_inode-385a56c57a51
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
block/bdev.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 9be8c3c683ae..b19cbcd6a4bf 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -799,16 +799,21 @@ static void bdev_claim_write_access(struct block_device *bdev, blk_mode_t mode)
bdev->bd_writers++;
}

-static void bdev_yield_write_access(struct block_device *bdev, blk_mode_t mode)
+static void bdev_yield_write_access(struct file *bdev_file, blk_mode_t mode)
{
+ struct block_device *bdev;
+
if (bdev_allow_write_mounted)
return;

+ bdev = file_bdev(bdev_file);
/* Yield exclusive or shared write access. */
- if (mode & BLK_OPEN_RESTRICT_WRITES)
- bdev_unblock_writes(bdev);
- else if (mode & BLK_OPEN_WRITE)
- bdev->bd_writers--;
+ if (mode & BLK_OPEN_WRITE) {
+ if (bdev_writes_blocked(bdev))
+ bdev_unblock_writes(bdev);
+ else
+ bdev->bd_writers--;
+ }
}

/**
@@ -1020,7 +1025,7 @@ void bdev_release(struct file *bdev_file)
sync_blockdev(bdev);

mutex_lock(&disk->open_mutex);
- bdev_yield_write_access(bdev, handle->mode);
+ bdev_yield_write_access(bdev_file, handle->mode);

if (handle->holder)
bd_end_claim(bdev, handle->holder);
--
2.43.0