diff mbox series

[v2,32/34] block: remove bdev_handle completely

Message ID 20240123-vfs-bdev-file-v2-32-adbd023e19cc@kernel.org (mailing list archive)
State New, archived
Headers show
Series Open block devices as files | expand

Commit Message

Christian Brauner Jan. 23, 2024, 1:26 p.m. UTC
We just need to use the holder to indicate whether a block device open
was exclusive or not. We did use to do that before but had to give that
up once we switched to struct bdev_handle. Before struct bdev_handle we
only stashed stuff in file->private_data if this was an exclusive open
but after struct bdev_handle we always set file->private_data to a
struct bdev_handle and so we had to use bdev_handle->mode or
bdev_handle->holder. Now that we don't use struct bdev_handle anymore we
can revert back to the old behavior.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c | 24 +++++++-----------------
 block/blk.h  |  5 -----
 block/fops.c | 18 +++++++++++-------
 3 files changed, 18 insertions(+), 29 deletions(-)

Comments

Christoph Hellwig Jan. 29, 2024, 4:50 p.m. UTC | #1
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Jan Kara Feb. 1, 2024, 11:20 a.m. UTC | #2
On Tue 23-01-24 14:26:49, Christian Brauner wrote:
> We just need to use the holder to indicate whether a block device open
> was exclusive or not. We did use to do that before but had to give that
> up once we switched to struct bdev_handle. Before struct bdev_handle we
> only stashed stuff in file->private_data if this was an exclusive open
> but after struct bdev_handle we always set file->private_data to a
> struct bdev_handle and so we had to use bdev_handle->mode or
> bdev_handle->holder. Now that we don't use struct bdev_handle anymore we
> can revert back to the old behavior.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Two small comments below.

> diff --git a/block/fops.c b/block/fops.c
> index f56bdfe459de..a0bff2c0d88d 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -569,7 +569,6 @@ static int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
>  blk_mode_t file_to_blk_mode(struct file *file)
>  {
>  	blk_mode_t mode = 0;
> -	struct bdev_handle *handle = file->private_data;
>  
>  	if (file->f_mode & FMODE_READ)
>  		mode |= BLK_OPEN_READ;
> @@ -579,8 +578,8 @@ blk_mode_t file_to_blk_mode(struct file *file)
>  	 * do_dentry_open() clears O_EXCL from f_flags, use handle->mode to
>  	 * determine whether the open was exclusive for already open files.
>  	 */
^^^ This comment needs update now...

> -	if (handle)
> -		mode |= handle->mode & BLK_OPEN_EXCL;
> +	if (file->private_data)
> +		mode |= BLK_OPEN_EXCL;
>  	else if (file->f_flags & O_EXCL)
>  		mode |= BLK_OPEN_EXCL;
>  	if (file->f_flags & O_NDELAY)
> @@ -601,12 +600,17 @@ static int blkdev_open(struct inode *inode, struct file *filp)
>  {
>  	struct block_device *bdev;
>  	blk_mode_t mode;
> -	void *holder;
>  	int ret;
>  
> +	/*
> +	 * Use the file private data to store the holder for exclusive opens.
> +	 * file_to_blk_mode relies on it being present to set BLK_OPEN_EXCL.
> +	 */
> +	if (filp->f_flags & O_EXCL)
> +		filp->private_data = filp;

Well, if we have O_EXCL in f_flags here, then file_to_blk_mode() on the
next line is going to do the right thing and set BLK_OPEN_EXCL even
without filp->private_data. So this shound't be needed AFAICT.

>  	mode = file_to_blk_mode(filp);
> -	holder = mode & BLK_OPEN_EXCL ? filp : NULL;
> -	ret = bdev_permission(inode->i_rdev, mode, holder);
> +	ret = bdev_permission(inode->i_rdev, mode, filp->private_data);
>  	if (ret)
>  		return ret;

								Honza
Christian Brauner Feb. 1, 2024, 4:18 p.m. UTC | #3
On Thu, Feb 01, 2024 at 12:20:08PM +0100, Jan Kara wrote:
> On Tue 23-01-24 14:26:49, Christian Brauner wrote:
> > We just need to use the holder to indicate whether a block device open
> > was exclusive or not. We did use to do that before but had to give that
> > up once we switched to struct bdev_handle. Before struct bdev_handle we
> > only stashed stuff in file->private_data if this was an exclusive open
> > but after struct bdev_handle we always set file->private_data to a
> > struct bdev_handle and so we had to use bdev_handle->mode or
> > bdev_handle->holder. Now that we don't use struct bdev_handle anymore we
> > can revert back to the old behavior.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Two small comments below.
> 
> > diff --git a/block/fops.c b/block/fops.c
> > index f56bdfe459de..a0bff2c0d88d 100644
> > --- a/block/fops.c
> > +++ b/block/fops.c
> > @@ -569,7 +569,6 @@ static int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
> >  blk_mode_t file_to_blk_mode(struct file *file)
> >  {
> >  	blk_mode_t mode = 0;
> > -	struct bdev_handle *handle = file->private_data;
> >  
> >  	if (file->f_mode & FMODE_READ)
> >  		mode |= BLK_OPEN_READ;
> > @@ -579,8 +578,8 @@ blk_mode_t file_to_blk_mode(struct file *file)
> >  	 * do_dentry_open() clears O_EXCL from f_flags, use handle->mode to
> >  	 * determine whether the open was exclusive for already open files.
> >  	 */
> ^^^ This comment needs update now...
> 
> > -	if (handle)
> > -		mode |= handle->mode & BLK_OPEN_EXCL;
> > +	if (file->private_data)
> > +		mode |= BLK_OPEN_EXCL;
> >  	else if (file->f_flags & O_EXCL)
> >  		mode |= BLK_OPEN_EXCL;
> >  	if (file->f_flags & O_NDELAY)
> > @@ -601,12 +600,17 @@ static int blkdev_open(struct inode *inode, struct file *filp)
> >  {
> >  	struct block_device *bdev;
> >  	blk_mode_t mode;
> > -	void *holder;
> >  	int ret;
> >  
> > +	/*
> > +	 * Use the file private data to store the holder for exclusive opens.
> > +	 * file_to_blk_mode relies on it being present to set BLK_OPEN_EXCL.
> > +	 */
> > +	if (filp->f_flags & O_EXCL)
> > +		filp->private_data = filp;
> 
> Well, if we have O_EXCL in f_flags here, then file_to_blk_mode() on the
> next line is going to do the right thing and set BLK_OPEN_EXCL even
> without filp->private_data. So this shound't be needed AFAICT.

Fixed.
diff mbox series

Patch

diff --git a/block/bdev.c b/block/bdev.c
index 9d96a43f198d..4b47003d8082 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -799,7 +799,7 @@  static void bdev_claim_write_access(struct block_device *bdev, blk_mode_t mode)
 		bdev->bd_writers++;
 }
 
-static void bdev_yield_write_access(struct file *bdev_file, blk_mode_t mode)
+static void bdev_yield_write_access(struct file *bdev_file)
 {
 	struct block_device *bdev;
 
@@ -810,7 +810,7 @@  static void bdev_yield_write_access(struct file *bdev_file, blk_mode_t mode)
 	/* Yield exclusive or shared write access. */
 	if (bdev_file->f_op == &def_blk_fops_restricted)
 		bdev_unblock_writes(bdev);
-	else if (mode & BLK_OPEN_WRITE)
+	else if (bdev_file->f_mode & FMODE_WRITE)
 		bdev->bd_writers--;
 }
 
@@ -838,16 +838,10 @@  static void bdev_yield_write_access(struct file *bdev_file, blk_mode_t mode)
 int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
 	      const struct blk_holder_ops *hops, struct file *bdev_file)
 {
-	struct bdev_handle *handle = kmalloc(sizeof(struct bdev_handle),
-					     GFP_KERNEL);
 	bool unblock_events = true;
 	struct gendisk *disk = bdev->bd_disk;
 	int ret;
 
-	handle = kmalloc(sizeof(struct bdev_handle), GFP_KERNEL);
-	if (!handle)
-		return -ENOMEM;
-
 	if (holder) {
 		mode |= BLK_OPEN_EXCL;
 		ret = bd_prepare_to_claim(bdev, holder, hops);
@@ -896,8 +890,6 @@  int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
 
 	if (unblock_events)
 		disk_unblock_events(disk);
-	handle->holder = holder;
-	handle->mode = mode;
 
 	/*
 	 * Preserve backwards compatibility and allow large file access
@@ -911,7 +903,7 @@  int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
 		bdev_file->f_mode |= FMODE_NOWAIT;
 	bdev_file->f_mapping = bdev->bd_inode->i_mapping;
 	bdev_file->f_wb_err = filemap_sample_wb_err(bdev_file->f_mapping);
-	bdev_file->private_data = handle;
+	bdev_file->private_data = holder;
 
 	return 0;
 put_module:
@@ -921,7 +913,6 @@  int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
 		bd_abort_claiming(bdev, holder);
 	mutex_unlock(&disk->open_mutex);
 	disk_unblock_events(disk);
-	kfree(handle);
 	return ret;
 }
 
@@ -1027,7 +1018,7 @@  EXPORT_SYMBOL(bdev_file_open_by_path);
 void bdev_release(struct file *bdev_file)
 {
 	struct block_device *bdev = file_bdev(bdev_file);
-	struct bdev_handle *handle = bdev_file->private_data;
+	void *holder = bdev_file->private_data;
 	struct gendisk *disk = bdev->bd_disk;
 
 	/*
@@ -1041,10 +1032,10 @@  void bdev_release(struct file *bdev_file)
 		sync_blockdev(bdev);
 
 	mutex_lock(&disk->open_mutex);
-	bdev_yield_write_access(bdev_file, handle->mode);
+	bdev_yield_write_access(bdev_file);
 
-	if (handle->holder)
-		bd_end_claim(bdev, handle->holder);
+	if (holder)
+		bd_end_claim(bdev, holder);
 
 	/*
 	 * Trigger event checking and tell drivers to flush MEDIA_CHANGE
@@ -1061,7 +1052,6 @@  void bdev_release(struct file *bdev_file)
 
 	module_put(disk->fops->owner);
 	blkdev_put_no_open(bdev);
-	kfree(handle);
 }
 
 /**
diff --git a/block/blk.h b/block/blk.h
index dfa958909c54..cce1ac0ff303 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -27,11 +27,6 @@  struct blk_flush_queue {
 	struct request		*flush_rq;
 };
 
-struct bdev_handle {
-	void *holder;
-	blk_mode_t mode;
-};
-
 bool is_flush_rq(struct request *req);
 
 struct blk_flush_queue *blk_alloc_flush_queue(int node, int cmd_size,
diff --git a/block/fops.c b/block/fops.c
index f56bdfe459de..a0bff2c0d88d 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -569,7 +569,6 @@  static int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
 blk_mode_t file_to_blk_mode(struct file *file)
 {
 	blk_mode_t mode = 0;
-	struct bdev_handle *handle = file->private_data;
 
 	if (file->f_mode & FMODE_READ)
 		mode |= BLK_OPEN_READ;
@@ -579,8 +578,8 @@  blk_mode_t file_to_blk_mode(struct file *file)
 	 * do_dentry_open() clears O_EXCL from f_flags, use handle->mode to
 	 * determine whether the open was exclusive for already open files.
 	 */
-	if (handle)
-		mode |= handle->mode & BLK_OPEN_EXCL;
+	if (file->private_data)
+		mode |= BLK_OPEN_EXCL;
 	else if (file->f_flags & O_EXCL)
 		mode |= BLK_OPEN_EXCL;
 	if (file->f_flags & O_NDELAY)
@@ -601,12 +600,17 @@  static int blkdev_open(struct inode *inode, struct file *filp)
 {
 	struct block_device *bdev;
 	blk_mode_t mode;
-	void *holder;
 	int ret;
 
+	/*
+	 * Use the file private data to store the holder for exclusive opens.
+	 * file_to_blk_mode relies on it being present to set BLK_OPEN_EXCL.
+	 */
+	if (filp->f_flags & O_EXCL)
+		filp->private_data = filp;
+
 	mode = file_to_blk_mode(filp);
-	holder = mode & BLK_OPEN_EXCL ? filp : NULL;
-	ret = bdev_permission(inode->i_rdev, mode, holder);
+	ret = bdev_permission(inode->i_rdev, mode, filp->private_data);
 	if (ret)
 		return ret;
 
@@ -614,7 +618,7 @@  static int blkdev_open(struct inode *inode, struct file *filp)
 	if (!bdev)
 		return -ENXIO;
 
-	ret = bdev_open(bdev, mode, holder, NULL, filp);
+	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
 	if (ret)
 		blkdev_put_no_open(bdev);
 	return ret;