Re: [PATCH v2 29/34] bdev: make struct bdev_handle private to the block layer

[Jan Kara <jack@suse.cz>]

On Tue 23-01-24 14:26:46, Christian Brauner wrote:
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Some comments below...

> @@ -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?

> @@ -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 :)

> +	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 <jack@suse.com>
SUSE Labs, CR