| Message ID | 0d6ec0b588b578b9f9aabef91b8ecdf9950b682a.1726488132.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: fix min_dio_alignment logic for getting device block size | expand |
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 > >
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. >
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. > > >
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; } }