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

[Christian Brauner <brauner@kernel.org>]

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

Indeed, I removed that.

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

Dropped.

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

Lack of caffeine? Fixed. Thanks for catching that.

> 
> 
> > +	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...

I rewrote this.

Total diff I applied is:

diff --git a/block/bdev.c b/block/bdev.c
index 0e8984884236..ba9dfa4648ca 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -893,12 +893,6 @@ int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
        handle->holder = holder;
        handle->mode = mode;

-       /*
-        * 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.
-        */
        bdev_file->f_flags |= O_LARGEFILE;
        bdev_file->f_mode |= FMODE_BUF_RASYNC | FMODE_CAN_ODIRECT;
        if (bdev_nowait(bdev))
@@ -960,7 +954,7 @@ struct file *bdev_file_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
        unsigned int flags;
        int ret;

-       ret = bdev_permission(dev, 0, holder);
+       ret = bdev_permission(dev, mode, holder);
        if (ret)
                return ERR_PTR(ret);

@@ -975,16 +969,14 @@ struct file *bdev_file_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
                blkdev_put_no_open(bdev);
                return bdev_file;
        }
-       bdev_file->f_mode &= ~FMODE_OPENED;
-
        ihold(bdev->bd_inode);
+
        ret = bdev_open(bdev, mode, holder, hops, bdev_file);
        if (ret) {
+               blkdev_put_no_open(bdev);
                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);
diff --git a/block/fops.c b/block/fops.c
index 81ff8c0ce32f..a1ba1a50ae77 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -622,7 +622,8 @@ static int blkdev_open(struct inode *inode, struct file *filp)

 static int blkdev_release(struct inode *inode, struct file *filp)
 {
-       bdev_release(filp->private_data);
+       if (filp->private_data)
+               bdev_release(filp->private_data);
        return 0;
 }