Re: [PATCH v2 29/34] bdev: make struct bdev_handle private to the block layer
From: Jan Kara <jack@suse.cz>
Date: 2024-02-01 10:54:24
Also in:
linux-fsdevel
On Tue 23-01-24 14:26:46, Christian Brauner wrote:
Signed-off-by: Christian Brauner <brauner@kernel.org>
Some comments below...
quoted hunk ↗ jump to hunk
@@ -795,15 +813,15 @@ static void bdev_yield_write_access(struct block_device *bdev, blk_mode_t mode) } /** - * bdev_open_by_dev - open a block device by device number - * @dev: device number of block device to open + * bdev_open - open a block device + * @bdev: block device to open * @mode: open mode (BLK_OPEN_*) * @holder: exclusive holder identifier * @hops: holder operations + * @bdev_file: file for the block device * - * Open the block device described by device number @dev. If @holder is not - * %NULL, the block device is opened with exclusive access. Exclusive opens may - * nest for the same @holder. + * Open the block device. If @holder is not %NULL, the block device is opened + * with exclusive access. Exclusive opens may nest for the same @holder. * * Use this interface ONLY if you really do not have anything better - i.e. when * you are behind a truly sucky interface and all you are given is a device
^^^ I guess this part of comment is stale now?
quoted hunk ↗ jump to hunk
@@ -902,7 +897,22 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, handle->bdev = bdev; handle->holder = holder; handle->mode = mode; - return handle; + + /* + * Preserve backwards compatibility and allow large file access + * even if userspace doesn't ask for it explicitly. Some mkfs + * binary needs it. We might want to drop this workaround + * during an unstable branch. + */
Heh, I think the sentense "We might want to drop this workaround during an unstable branch." does not need to be moved as well :)
quoted hunk ↗ jump to hunk
+ bdev_file->f_flags |= O_LARGEFILE; + bdev_file->f_mode |= FMODE_BUF_RASYNC | FMODE_CAN_ODIRECT; + if (bdev_nowait(bdev)) + bdev_file->f_mode |= FMODE_NOWAIT; + bdev_file->f_mapping = handle->bdev->bd_inode->i_mapping; + bdev_file->f_wb_err = filemap_sample_wb_err(bdev_file->f_mapping); + bdev_file->private_data = handle; + + return 0; put_module: module_put(disk->fops->owner); abort_claiming:@@ -910,11 +920,8 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, bd_abort_claiming(bdev, holder); mutex_unlock(&disk->open_mutex); disk_unblock_events(disk); -put_blkdev: - blkdev_put_no_open(bdev); -free_handle: kfree(handle); - return ERR_PTR(ret); + return ret; } static unsigned blk_to_file_flags(blk_mode_t mode)@@ -954,29 +961,35 @@ struct file *bdev_file_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, const struct blk_holder_ops *hops) { struct file *bdev_file; - struct bdev_handle *handle; + struct block_device *bdev; unsigned int flags; + int ret; - handle = bdev_open_by_dev(dev, mode, holder, hops); - if (IS_ERR(handle)) - return ERR_CAST(handle); + ret = bdev_permission(dev, 0, holder);
^^ Maybe I'm missing something but why do you pass 0 mode here?
+ if (ret)
+ return ERR_PTR(ret);
+
+ bdev = blkdev_get_no_open(dev);
+ if (!bdev)
+ return ERR_PTR(-ENXIO);
flags = blk_to_file_flags(mode);
- bdev_file = alloc_file_pseudo_noaccount(handle->bdev->bd_inode,
+ bdev_file = alloc_file_pseudo_noaccount(bdev->bd_inode,
blockdev_mnt, "", flags | O_LARGEFILE, &def_blk_fops);
if (IS_ERR(bdev_file)) {
- bdev_release(handle);
+ blkdev_put_no_open(bdev);
return bdev_file;
}
- ihold(handle->bdev->bd_inode);
+ bdev_file->f_mode &= ~FMODE_OPENED;Hum, why do you need these games with FMODE_OPENED? I suspect you want to influence fput() behavior but then AFAICT we will leak dentry, mnt, etc. on error? If this is indeed needed, it deserves a comment...
- bdev_file->f_mode |= FMODE_BUF_RASYNC | FMODE_CAN_ODIRECT;
- if (bdev_nowait(handle->bdev))
- bdev_file->f_mode |= FMODE_NOWAIT;
-
- bdev_file->f_mapping = handle->bdev->bd_inode->i_mapping;
- bdev_file->f_wb_err = filemap_sample_wb_err(bdev_file->f_mapping);
- bdev_file->private_data = handle;
+ ihold(bdev->bd_inode);
+ ret = bdev_open(bdev, mode, holder, hops, bdev_file);
+ if (ret) {
+ fput(bdev_file);
+ return ERR_PTR(ret);
+ }
+ /* Now that thing is opened. */
+ bdev_file->f_mode |= FMODE_OPENED;
return bdev_file;
}
EXPORT_SYMBOL(bdev_file_open_by_dev);Honza -- Jan Kara [off-list ref] SUSE Labs, CR