[PATCH] fstests: fix min_dio_alignment logic for getting device block size

[PATCH] fstests: fix min_dio_alignment logic for getting device block size

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we failed to get the dio alignment from statx we try to get the
device's block size using the BLKSSZGET ioctl, however we failed to
return it because we don't check if the ioctl succeeded (returned 0).
Furthermore in case the ioctl returned an error, we end up returning an
undefined value since the 'logical_block_size' variable ends up not
being initialized.

This was causing some tests to be skipped on btrfs after commit
ee799a0cf1d4 ("replace _min_dio_alignment with calls to
src/min_dio_alignment"), like generic/240 for example:

  $ ./check generic/240
  FSTYP         -- btrfs
  PLATFORM      -- Linux/x86_64 debian0 6.11.0-rc7-btrfs-next-174+ #1 SMP PREEMPT_DYNAMIC Tue Sep 10 17:11:38 WEST 2024
  MKFS_OPTIONS  -- /dev/sdc
  MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1

  generic/240 1s ... [not run] fs block size must be larger than the device block size.  fs block size: 4096, device block size: 4096
  Ran: generic/240
  Not run: generic/240
  Passed all 1 tests

Where before that commit the test ran.

Fix this by checking that the ioctl succeeded.

Fixes: 0e5f196d0a6a ("add a new min_dio_alignment helper")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 src/min_dio_alignment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/min_dio_alignment.c b/src/min_dio_alignment.c
index 131f6023..5a7c7d9f 100644
--- a/src/min_dio_alignment.c
+++ b/src/min_dio_alignment.c
@@ -42,7 +42,7 @@ static int min_dio_alignment(const char *mntpnt, const char *devname)
 		if (dev_fd > 0 &&
 		    fstat(dev_fd, &st) == 0 &&
 		    S_ISBLK(st.st_mode) &&
-		    ioctl(dev_fd, BLKSSZGET, &logical_block_size)) {
+		    ioctl(dev_fd, BLKSSZGET, &logical_block_size) == 0) {
 			return logical_block_size;
 		}
 	}
-- 
2.43.0

Re: [PATCH] fstests: fix min_dio_alignment logic for getting device block size

[Darrick J. Wong]

On Mon, Sep 16, 2024 at 01:03:12PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we failed to get the dio alignment from statx we try to get the
> device's block size using the BLKSSZGET ioctl, however we failed to
> return it because we don't check if the ioctl succeeded (returned 0).
> Furthermore in case the ioctl returned an error, we end up returning an
> undefined value since the 'logical_block_size' variable ends up not
> being initialized.
> 
> This was causing some tests to be skipped on btrfs after commit
> ee799a0cf1d4 ("replace _min_dio_alignment with calls to
> src/min_dio_alignment"), like generic/240 for example:
> 
>   $ ./check generic/240
>   FSTYP         -- btrfs
>   PLATFORM      -- Linux/x86_64 debian0 6.11.0-rc7-btrfs-next-174+ #1 SMP PREEMPT_DYNAMIC Tue Sep 10 17:11:38 WEST 2024
>   MKFS_OPTIONS  -- /dev/sdc
>   MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
> 
>   generic/240 1s ... [not run] fs block size must be larger than the device block size.  fs block size: 4096, device block size: 4096
>   Ran: generic/240
>   Not run: generic/240
>   Passed all 1 tests
> 
> Where before that commit the test ran.
> 
> Fix this by checking that the ioctl succeeded.
> 
> Fixes: 0e5f196d0a6a ("add a new min_dio_alignment helper")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Looks good, sorry for missing that during review.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  src/min_dio_alignment.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/min_dio_alignment.c b/src/min_dio_alignment.c
> index 131f6023..5a7c7d9f 100644
> --- a/src/min_dio_alignment.c
> +++ b/src/min_dio_alignment.c
> @@ -42,7 +42,7 @@ static int min_dio_alignment(const char *mntpnt, const char *devname)
>  		if (dev_fd > 0 &&
>  		    fstat(dev_fd, &st) == 0 &&
>  		    S_ISBLK(st.st_mode) &&
> -		    ioctl(dev_fd, BLKSSZGET, &logical_block_size)) {
> +		    ioctl(dev_fd, BLKSSZGET, &logical_block_size) == 0) {
>  			return logical_block_size;
>  		}
>  	}
> -- 
> 2.43.0
> 
> 

Re: [PATCH] fstests: fix min_dio_alignment logic for getting device block size

[Christoph Hellwig]

On Mon, Sep 16, 2024 at 01:03:12PM +0100, fdmanana@kernel.org wrote:
> Fix this by checking that the ioctl succeeded.

The fixes look good:

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

What file do you manage to open but fail BLKSSZGET on here?  This smells
like we have an issue in high level logic.

And btrfs really should implement STATX_DIOALIGN with it's multi-device
setup that doesn't make the fallback very useful.

> 

Re: [PATCH] fstests: fix min_dio_alignment logic for getting device block size

[Filipe Manana]

On Tue, Sep 17, 2024 at 6:20 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Sep 16, 2024 at 01:03:12PM +0100, fdmanana@kernel.org wrote:
> > Fix this by checking that the ioctl succeeded.
>
> The fixes look good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> What file do you manage to open but fail BLKSSZGET on here?  This smells
> like we have an issue in high level logic.

I haven't hit any case where the ioctl failed. I was just saying that
in case it fails, we may return an uninitialized/undefined value.


>
> And btrfs really should implement STATX_DIOALIGN with it's multi-device
> setup that doesn't make the fallback very useful.
>
> >