Re: [PATCHv5 08/11] iov: introduce iov_iter_aligned

Re: [PATCHv5 08/11] iov: introduce iov_iter_aligned

[Christoph Hellwig]

On Tue, May 31, 2022 at 12:11:34PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The existing iov_iter_alignment() function returns the logical OR of
> address and length. For cases where address and length need to be
> considered separately, introduce a helper function that a caller can
> specificy length and address masks that indicate if the iov is
> unaligned.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 09/11] block: introduce bdev_iter_is_aligned helper

[Eric Biggers]

On Tue, May 31, 2022 at 12:11:35PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Provide a convenient function for this repeatable coding pattern.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  include/linux/blkdev.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 834b981ef01b..583cdeb8895d 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1370,6 +1370,13 @@ static inline unsigned int bdev_dma_alignment(struct block_device *bdev)
>  	return queue_dma_alignment(bdev_get_queue(bdev));
>  }
>  
> +static inline bool bvev_iter_is_aligned(struct block_device *bdev,
> +					struct iov_iter *iter)
> +{
> +	return iov_iter_is_aligned(iter, bdev_dma_alignment(bdev),
> +				   bdev_logical_block_size(bdev) - 1);
> +}

"bdev", not "bvev".

- Eric

Re: [PATCHv5 09/11] block: introduce bdev_iter_is_aligned helper

[Christoph Hellwig]

Modulo the naming typo:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 10/11] block: relax direct io memory alignment

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 11/11] fs: add support for dma aligned direct-io

[Christoph Hellwig]

Looks good with a s/fs/iomap/ in the subject:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 07/11] block/bounce: count bytes instead of sectors

[Eric Biggers]

On Tue, May 31, 2022 at 12:11:33PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Individual bv_len's may not be a sector size.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
> v4->v5:
> 
>   Updated comment (Christoph)
> 
>  block/bounce.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/block/bounce.c b/block/bounce.c
> index 8f7b6fe3b4db..fbadf179601f 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -205,19 +205,26 @@ void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig)
>  	int rw = bio_data_dir(*bio_orig);
>  	struct bio_vec *to, from;
>  	struct bvec_iter iter;
> -	unsigned i = 0;
> +	unsigned i = 0, bytes = 0;
>  	bool bounce = false;
> -	int sectors = 0;
> +	int sectors;
>  
>  	bio_for_each_segment(from, *bio_orig, iter) {
>  		if (i++ < BIO_MAX_VECS)
> -			sectors += from.bv_len >> 9;
> +			bytes += from.bv_len;
>  		if (PageHighMem(from.bv_page))
>  			bounce = true;
>  	}
>  	if (!bounce)
>  		return;
>  
> +	/*
> +	 * Individual bvecs may not be logical block aligned. Round down
> +	 * the split size so that each bio is properly sector size aligned,
> +	 * even if we do not use the full hardware limits.
> +	 */

Please write "might not" instead of "may not", since "may not" is ambiguous; it
sometimes means "are not allowed to".  Likewise in other patches.

"Sector size" is ambiguous as well.  I think you mean "logical block size"?

- Eric

Re: [PATCHv5 00/11] direct-io dma alignment

[Eric Biggers]

On Tue, May 31, 2022 at 12:11:26PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The most significant change from v4 is the alignment is now checked
> prior to building the bio. This gets the expected EINVAL error for
> misaligned userspace iovecs in all cases now (Eric Biggers).
> 
> I've removed the legacy fs change, so only iomap filesystems get to use
> this alignement capability (Christoph Hellwig).
> 
> The block fops check for alignment returns a bool now (Damien).
> 
> Adjusted some comments, docs, and other minor style issues.
> 
> Reviews added for unchanged or trivially changed patches, removed
> reviews for ones that changed more significantly.
> 
> As before, I tested using 'fio' with forced misaligned user buffers on
> raw block, xfs, and ext4 (example raw block profile below).
> 

I still don't think you've taken care of all the assumptions that bv_len is a
multiple of logical block size, or at least SECTOR_SIZE.  Try this:

	git grep -E 'bv_len (>>|/)'

Also:

	git grep '<.*bv_len;'

Also take a look at bio_for_each_segment(), specifically how iter->bi_sector is
updated.

- Eric

Re: [PATCHv5 00/11] direct-io dma alignment

[Keith Busch]

On Wed, Jun 01, 2022 at 12:11:40AM -0700, Eric Biggers wrote:
> On Tue, May 31, 2022 at 12:11:26PM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The most significant change from v4 is the alignment is now checked
> > prior to building the bio. This gets the expected EINVAL error for
> > misaligned userspace iovecs in all cases now (Eric Biggers).
> > 
> > I've removed the legacy fs change, so only iomap filesystems get to use
> > this alignement capability (Christoph Hellwig).
> > 
> > The block fops check for alignment returns a bool now (Damien).
> > 
> > Adjusted some comments, docs, and other minor style issues.
> > 
> > Reviews added for unchanged or trivially changed patches, removed
> > reviews for ones that changed more significantly.
> > 
> > As before, I tested using 'fio' with forced misaligned user buffers on
> > raw block, xfs, and ext4 (example raw block profile below).
> > 
> 
> I still don't think you've taken care of all the assumptions that bv_len is a
> multiple of logical block size, or at least SECTOR_SIZE.  Try this:
> 
> 	git grep -E 'bv_len (>>|/)'

There are only 8 drivers that set the request_queue's dma alignment, which are
the only ones that could be affected from this patch series. The drivers
returned from the above don't set dma alignment, so they're fine to assume
those lengths.

I don't think the above query captures enough since it misses things like
nfhd_submit_bio() that shifts 9 on the following line. Not that that example
matters either for the same reason.
 
> Also:
> 
> 	git grep '<.*bv_len;'
>
> Also take a look at bio_for_each_segment(), specifically how iter->bi_sector is
> updated.

I'm not finding any driver user of this macro that's set the dma alignment
where this would break. They either never set it, or they're stacking drivers
that always get the safe default.

Outside drivers, blk-integrity doesn't operate on sector lengths, so that's
fine, and blk-crypto would prevent such unalignment much earlier. And btrfs
bounces this type of direct IO, so that's also fine.

Even if we assume all the existing users are safe, I suppose we could say this
type of assumption is potentially fragile. For example, I think drivers like
pmem or null_blk could readily reduce their queue's dma alignment limit, but
that may break their current usage.

Re: [PATCHv5 00/11] direct-io dma alignment

[Keith Busch]

On Wed, Jun 01, 2022 at 08:28:31AM -0600, Keith Busch wrote:
> On Wed, Jun 01, 2022 at 12:11:40AM -0700, Eric Biggers wrote:
> > I still don't think you've taken care of all the assumptions that bv_len is a
> > multiple of logical block size, or at least SECTOR_SIZE.  Try this:
> > 
> > 	git grep -E 'bv_len (>>|/)'
> 
> There are only 8 drivers that set the request_queue's dma alignment, which are
> the only ones that could be affected from this patch series.

It's actually even simpler to audit than that. Of the 8 drivers that explicitly
set dma alignment, only 3 set it to something smaller than a sector size. None
of them assume any particular bv_len, so I think we're fine.

Re: [PATCHv5 00/11] direct-io dma alignment

[Keith Busch]

On Wed, Jun 01, 2022 at 10:12:26AM -0600, Keith Busch wrote:
> On Wed, Jun 01, 2022 at 08:28:31AM -0600, Keith Busch wrote:
> > On Wed, Jun 01, 2022 at 12:11:40AM -0700, Eric Biggers wrote:
> > > I still don't think you've taken care of all the assumptions that bv_len is a
> > > multiple of logical block size, or at least SECTOR_SIZE.  Try this:
> > > 
> > > 	git grep -E 'bv_len (>>|/)'
> > 
> > There are only 8 drivers that set the request_queue's dma alignment, which are
> > the only ones that could be affected from this patch series.
> 
> It's actually even simpler to audit than that. Of the 8 drivers that explicitly
> set dma alignment, only 3 set it to something smaller than a sector size. None
> of them assume any particular bv_len, so I think we're fine.

Eric,

Do you have any more concerns on this area? I'm reasonably confident this is
safe with all the existing users, and I have a new series ready to go with the
trivial fix-ups from the last round. I don't want to post it, though, if you
think I've missed something.

Thanks,
Keith

Re: [PATCHv5 01/11] block: fix infinite loop for invalid zone append

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCHv5 05/11] block: add a helper function for dio alignment

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 05/11] block: add a helper function for dio alignment

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCHv5 06/11] block/merge: count bytes instead of sectors

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>