diff mbox series

[f2fs-dev,01/30] block: also call ->open for incremental partition opens

Message ID 20230608110258.189493-2-hch@lst.de (mailing list archive)
State Accepted
Commit 9d1c92872e7082f100f629a58b32fa0214aa1aec
Headers show
Series [f2fs-dev,01/30] block: also call ->open for incremental partition opens | expand

Commit Message

Christoph Hellwig June 8, 2023, 11:02 a.m. UTC
For whole devices ->open is called for each open, but for partitions it
is only called on the first open of a partition, e.g.:

  open("/dev/vdb", ...)
  open("/dev/vdb", ...)
    - 2 call to ->open

  open("/dev/vdb1", ...)
  open("/dev/vdb", ...)
    - 2 call to ->open

  open("/dev/vdb", ...)
  open("/dev/vdb", ...)
    - just open call to ->open

This is problematic as various block drivers look at open flags and
might not do all the required setup if the earlier open was with an
odd flag like O_NDELAY or the magic 3 ioctl-only open mode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Phillip Potter <phil@philpotter.co.uk>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Acked-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

patchwork-bot+f2fs@kernel.org July 6, 2023, 12:18 a.m. UTC | #1
Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jens Axboe <axboe@kernel.dk>:

On Thu,  8 Jun 2023 13:02:29 +0200 you wrote:
> For whole devices ->open is called for each open, but for partitions it
> is only called on the first open of a partition, e.g.:
> 
>   open("/dev/vdb", ...)
>   open("/dev/vdb", ...)
>     - 2 call to ->open
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,01/30] block: also call ->open for incremental partition opens
    https://git.kernel.org/jaegeuk/f2fs/c/9d1c92872e70
  - [f2fs-dev,02/30] cdrom: remove the unused bdev argument to cdrom_open
    https://git.kernel.org/jaegeuk/f2fs/c/764b83100b9a
  - [f2fs-dev,03/30] cdrom: remove the unused mode argument to cdrom_ioctl
    https://git.kernel.org/jaegeuk/f2fs/c/473399b50de1
  - [f2fs-dev,04/30] cdrom: remove the unused cdrom_close_write release code
    https://git.kernel.org/jaegeuk/f2fs/c/a4cec8bc14c0
  - [f2fs-dev,05/30] cdrom: track if a cdrom_device_info was opened for data
    https://git.kernel.org/jaegeuk/f2fs/c/8cdf433e2b8e
  - [f2fs-dev,06/30] cdrom: remove the unused mode argument to cdrom_release
    https://git.kernel.org/jaegeuk/f2fs/c/7ae24fcee992
  - [f2fs-dev,07/30] block: pass a gendisk on bdev_check_media_change
    https://git.kernel.org/jaegeuk/f2fs/c/444aa2c58cb3
  - [f2fs-dev,08/30] block: pass a gendisk to ->open
    https://git.kernel.org/jaegeuk/f2fs/c/d32e2bf83791
  - [f2fs-dev,09/30] block: remove the unused mode argument to ->release
    https://git.kernel.org/jaegeuk/f2fs/c/ae220766d87c
  - [f2fs-dev,10/30] block: rename blkdev_close to blkdev_release
    https://git.kernel.org/jaegeuk/f2fs/c/7ee34cbc291a
  - [f2fs-dev,11/30] swsusp: don't pass a stack address to blkdev_get_by_path
    https://git.kernel.org/jaegeuk/f2fs/c/c889d0793d9d
  - [f2fs-dev,12/30] bcache: don't pass a stack address to blkdev_get_by_path
    https://git.kernel.org/jaegeuk/f2fs/c/29499ab060fe
  - [f2fs-dev,13/30] rnbd-srv: don't pass a holder for non-exclusive blkdev_get_by_path
    https://git.kernel.org/jaegeuk/f2fs/c/5ee607675deb
  - [f2fs-dev,14/30] btrfs: don't pass a holder for non-exclusive blkdev_get_by_path
    https://git.kernel.org/jaegeuk/f2fs/c/2ef789288afd
  - [f2fs-dev,15/30] block: use the holder as indication for exclusive opens
    https://git.kernel.org/jaegeuk/f2fs/c/2736e8eeb0cc
  - [f2fs-dev,16/30] block: add a sb_open_mode helper
    https://git.kernel.org/jaegeuk/f2fs/c/3f0b3e785e8b
  - [f2fs-dev,17/30] fs: remove sb->s_mode
    https://git.kernel.org/jaegeuk/f2fs/c/81b1fb7d17c0
  - [f2fs-dev,18/30] scsi: replace the fmode_t argument to scsi_cmd_allowed with a simple bool
    https://git.kernel.org/jaegeuk/f2fs/c/5f4eb9d5413f
  - [f2fs-dev,19/30] scsi: replace the fmode_t argument to scsi_ioctl with a simple bool
    https://git.kernel.org/jaegeuk/f2fs/c/2e80089c1824
  - [f2fs-dev,20/30] scsi: replace the fmode_t argument to ->sg_io_fn with a simple bool
    https://git.kernel.org/jaegeuk/f2fs/c/1991299e49fa
  - [f2fs-dev,21/30] nvme: replace the fmode_t argument to the nvme ioctl handlers with a simple bool
    https://git.kernel.org/jaegeuk/f2fs/c/7d9d7d59d44b
  - [f2fs-dev,22/30] mtd: block: use a simple bool to track open for write
    https://git.kernel.org/jaegeuk/f2fs/c/658afed19cee
  - [f2fs-dev,23/30] rnbd-srv: replace sess->open_flags with a "bool readonly"
    https://git.kernel.org/jaegeuk/f2fs/c/99b07780814e
  - [f2fs-dev,24/30] ubd: remove commented out code in ubd_open
    https://git.kernel.org/jaegeuk/f2fs/c/bd6abfc8e789
  - [f2fs-dev,25/30] block: move a few internal definitions out of blkdev.h
    https://git.kernel.org/jaegeuk/f2fs/c/cfb425761c79
  - [f2fs-dev,26/30] block: remove unused fmode_t arguments from ioctl handlers
    https://git.kernel.org/jaegeuk/f2fs/c/5e4ea834676e
  - [f2fs-dev,27/30] block: replace fmode_t with a block-specific type for block open flags
    https://git.kernel.org/jaegeuk/f2fs/c/05bdb9965305
  - [f2fs-dev,28/30] block: always use I_BDEV on file->f_mapping->host to find the bdev
    https://git.kernel.org/jaegeuk/f2fs/c/4e762d862344
  - [f2fs-dev,29/30] block: store the holder in file->private_data
    https://git.kernel.org/jaegeuk/f2fs/c/ee3249a8ce78
  - [f2fs-dev,30/30] fs: remove the now unused FMODE_* flags
    https://git.kernel.org/jaegeuk/f2fs/c/0733ad800291

You are awesome, thank you!
Al Viro Aug. 25, 2023, 2:44 a.m. UTC | #2
On Thu, Jun 08, 2023 at 01:02:29PM +0200, Christoph Hellwig wrote:

> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -683,9 +683,6 @@ static int blkdev_get_part(struct block_device *part, fmode_t mode)
>  	struct gendisk *disk = part->bd_disk;
>  	int ret;
>  
> -	if (atomic_read(&part->bd_openers))
> -		goto done;
> -
>  	ret = blkdev_get_whole(bdev_whole(part), mode);
>  	if (ret)
>  		return ret;
> @@ -694,9 +691,10 @@ static int blkdev_get_part(struct block_device *part, fmode_t mode)
>  	if (!bdev_nr_sectors(part))
>  		goto out_blkdev_put;
>  
> -	disk->open_partitions++;
> -	set_init_blocksize(part);
> -done:
> +	if (!atomic_read(&part->bd_openers)) {
> +		disk->open_partitions++;
> +		set_init_blocksize(part);
> +	}

[with apologies for very late (and tangential) reply]

That got me curious about the ->bd_openers - do we need it atomic?
Most of the users (and all places that do modifications) are
under ->open_mutex; the only exceptions are
	* early sync logics in blkdev_put(); it's explicitly racy -
see the comment there.
	* callers of disk_openers() in loop and nbd (the ones in
zram are under ->open_mutex).  There's driver-private exclusion
around those, but in any case - READ_ONCE() is no worse than
atomic_read() in those cases.

Is there something subtle I'm missing here?
Christoph Hellwig Aug. 28, 2023, 12:09 p.m. UTC | #3
On Fri, Aug 25, 2023 at 03:44:57AM +0100, Al Viro wrote:
> That got me curious about the ->bd_openers - do we need it atomic?
> Most of the users (and all places that do modifications) are
> under ->open_mutex; the only exceptions are
> 	* early sync logics in blkdev_put(); it's explicitly racy -
> see the comment there.
> 	* callers of disk_openers() in loop and nbd (the ones in
> zram are under ->open_mutex).  There's driver-private exclusion
> around those, but in any case - READ_ONCE() is no worse than
> atomic_read() in those cases.
> 
> Is there something subtle I'm missing here?

No.  When I had to add unlocked readers I did the READ_ONCE initially,
but reviewers though the atomic_t would be better.  I didn't really feel
like arguing so went with this version.
diff mbox series

Patch

diff --git a/block/bdev.c b/block/bdev.c
index 5c46ff10770638..981f6135795138 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -683,9 +683,6 @@  static int blkdev_get_part(struct block_device *part, fmode_t mode)
 	struct gendisk *disk = part->bd_disk;
 	int ret;
 
-	if (atomic_read(&part->bd_openers))
-		goto done;
-
 	ret = blkdev_get_whole(bdev_whole(part), mode);
 	if (ret)
 		return ret;
@@ -694,9 +691,10 @@  static int blkdev_get_part(struct block_device *part, fmode_t mode)
 	if (!bdev_nr_sectors(part))
 		goto out_blkdev_put;
 
-	disk->open_partitions++;
-	set_init_blocksize(part);
-done:
+	if (!atomic_read(&part->bd_openers)) {
+		disk->open_partitions++;
+		set_init_blocksize(part);
+	}
 	atomic_inc(&part->bd_openers);
 	return 0;
 
@@ -709,10 +707,10 @@  static void blkdev_put_part(struct block_device *part, fmode_t mode)
 {
 	struct block_device *whole = bdev_whole(part);
 
-	if (!atomic_dec_and_test(&part->bd_openers))
-		return;
-	blkdev_flush_mapping(part);
-	whole->bd_disk->open_partitions--;
+	if (atomic_dec_and_test(&part->bd_openers)) {
+		blkdev_flush_mapping(part);
+		whole->bd_disk->open_partitions--;
+	}
 	blkdev_put_whole(whole, mode);
 }