Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Mon, 11 Dec 2023 16:27:30 +0530
From:   Ojaswin Mujoo <ojaswin@linux.ibm.com>
To:     John Garry <john.g.garry@oracle.com>
Cc:     linux-ext4@vger.kernel.org, "Theodore Ts'o" <tytso@mit.edu>,
        Ritesh Harjani <ritesh.list@gmail.com>,
        linux-kernel@vger.kernel.org,
        "Darrick J . Wong" <djwong@kernel.org>,
        linux-block@vger.kernel.org, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, dchinner@redhat.com
Subject: Re: [RFC 5/7] block: export blkdev_atomic_write_valid() and refactor
 api
Message-ID: <ZXbrGvkJRIJmRtex@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <cover.1701339358.git.ojaswin@linux.ibm.com>
 <b53609d0d4b97eb9355987ac5ec03d4e89293b43.1701339358.git.ojaswin@linux.ibm.com>
 <cc43b1ba-e9ea-4ff1-b616-be3c11960eea@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cc43b1ba-e9ea-4ff1-b616-be3c11960eea@oracle.com>
Precedence: bulk

On Fri, Dec 01, 2023 at 10:47:59AM +0000, John Garry wrote:
> On 30/11/2023 13:53, Ojaswin Mujoo wrote:
> > Export the blkdev_atomic_write_valid() function so that other filesystems
> > can call it as a part of validating the atomic write operation.
> > 
> > Further, refactor the api to accept a len argument instead of iov_iter to
> > make it easier to call from other places.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> I was actually thinking of moving this functionality to vfs and maybe also
> calling earlier in write path, as the code is really common to blkdev and
> FSes.

This makes sense. The code to make sure the underlying device
will be able to support this atomic write can be moved higher up in vfs.
And then each fs can do extra fs-specific checks in their code.

> 
> However, Christoph Hellwig was not so happy about current interface with
> power-of-2 requirement et al, so I was going to wait until that discussion
> is concluded before deciding.

Got it, I'll leave this bit to you then :) 

> 
> Thanks,
> John
> 
> > ---
> >   block/fops.c           | 18 ++++++++++--------
> >   include/linux/blkdev.h |  2 ++
> >   2 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/fops.c b/block/fops.c
> > index 516669ad69e5..5dae95c49720 100644
> > --- a/block/fops.c
> > +++ b/block/fops.c
> > @@ -41,8 +41,7 @@ static bool blkdev_dio_unaligned(struct block_device *bdev, loff_t pos,
> >   		!bdev_iter_is_aligned(bdev, iter);
> >   }
> > -static bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos,
> > -			      struct iov_iter *iter)
> > +bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos, size_t len)
> >   {
> >   	unsigned int atomic_write_unit_min_bytes =
> >   			queue_atomic_write_unit_min_bytes(bdev_get_queue(bdev));
> > @@ -53,16 +52,17 @@ static bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos,
> >   		return false;
> >   	if (pos % atomic_write_unit_min_bytes)
> >   		return false;
> > -	if (iov_iter_count(iter) % atomic_write_unit_min_bytes)
> > +	if (len % atomic_write_unit_min_bytes)
> >   		return false;
> > -	if (!is_power_of_2(iov_iter_count(iter)))
> > +	if (!is_power_of_2(len))
> >   		return false;
> > -	if (iov_iter_count(iter) > atomic_write_unit_max_bytes)
> > +	if (len > atomic_write_unit_max_bytes)
> >   		return false;
> > -	if (pos % iov_iter_count(iter))
> > +	if (pos % len)
> >   		return false;
> >   	return true;
> >   }
> > +EXPORT_SYMBOL_GPL(blkdev_atomic_write_valid);
> >   #define DIO_INLINE_BIO_VECS 4
> > @@ -81,7 +81,8 @@ static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
> >   	if (blkdev_dio_unaligned(bdev, pos, iter))
> >   		return -EINVAL;
> > -	if (atomic_write && !blkdev_atomic_write_valid(bdev, pos, iter))
> > +	if (atomic_write &&
> > +	    !blkdev_atomic_write_valid(bdev, pos, iov_iter_count(iter)))
> >   		return -EINVAL;
> >   	if (nr_pages <= DIO_INLINE_BIO_VECS)
> > @@ -348,7 +349,8 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
> >   	if (blkdev_dio_unaligned(bdev, pos, iter))
> >   		return -EINVAL;
> > -	if (atomic_write && !blkdev_atomic_write_valid(bdev, pos, iter))
> > +	if (atomic_write &&
> > +	    !blkdev_atomic_write_valid(bdev, pos, iov_iter_count(iter)))
> >   		return -EINVAL;
> >   	if (iocb->ki_flags & IOCB_ALLOC_CACHE)
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index f70988083734..5a3124fc191f 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -1566,6 +1566,8 @@ static inline int early_lookup_bdev(const char *pathname, dev_t *dev)
> >   int freeze_bdev(struct block_device *bdev);
> >   int thaw_bdev(struct block_device *bdev);
> > +bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos, size_t len);
> > +
> >   struct io_comp_batch {
> >   	struct request *req_list;
> >   	bool need_ts;
>