| Message ID | 20230608110258.189493-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [01/30] block: also call ->open for incremental partition opens | expand |
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!
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?
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 --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); }
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(-)