[PATCHSET 0/2] xfs_db: use directio for filesystem access

[PATCHSET 0/2] xfs_db: use directio for filesystem access

[Darrick J. Wong]

Hi all,

xfs_db doesn't even try to use directio to access block devices.  This
is rather strange, since mkfs, copy, and repair use it.  Modify xfs_db
to set LIBXFS_DIRECT so that we don't have to deal with the bdev
pagecache being out of sync with what the kernel might have written
directly to the block device.

While we're at it, if we're using directio to the block device, don't
fail if we can't set the bufferhead size to the sectorsize, because
we're not using the pagecache anyway.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been running on the djcloud for months with no problems.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=db-use-directio
---
 db/init.c     |    1 +
 libxfs/init.c |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

[PATCH 1/2] libxfs: make platform_set_blocksize optional with directio

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If we're accessing the block device with directio (and hence bypassing
the page cache), then don't fail on BLKBSZSET not working.  We don't
care what happens to the pagecache bufferheads.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/init.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


diff --git a/libxfs/init.c b/libxfs/init.c
index fda36ba0f7d..ce6e62cde94 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -125,10 +125,14 @@ libxfs_device_open(char *path, int creat, int xflags, int setblksize)
 	}
 
 	if (!readonly && setblksize && (statb.st_mode & S_IFMT) == S_IFBLK) {
-		if (setblksize == 1)
+		if (setblksize == 1) {
 			/* use the default blocksize */
 			(void)platform_set_blocksize(fd, path, statb.st_rdev, XFS_MIN_SECTORSIZE, 0);
-		else {
+		} else if (dio) {
+			/* try to use the given explicit blocksize */
+			(void)platform_set_blocksize(fd, path, statb.st_rdev,
+					setblksize, 0);
+		} else {
 			/* given an explicit blocksize to use */
 			if (platform_set_blocksize(fd, path, statb.st_rdev, setblksize, 1))
 			    exit(1);

[PATCH 2/2] xfs_db: use directio for device access

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

XFS and tools (mkfs, copy, repair) don't generally rely on the block
device page cache, preferring instead to use directio.  For whatever
reason, the debugger was never made to do this, but let's do that now.

This should eliminate the weird fstests failures resulting from
udev/blkid pinning a cache page while the unmounting filesystem writes
to the superblock such that xfs_db finds the stale pagecache instead of
the post-unmount superblock.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/init.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/db/init.c b/db/init.c
index eec65d0884d..4599cc00d71 100644
--- a/db/init.c
+++ b/db/init.c
@@ -96,6 +96,7 @@ init(
 		x.volname = fsdevice;
 	else
 		x.dname = fsdevice;
+	x.isdirect = LIBXFS_DIRECT;
 
 	x.bcache_flags = CACHE_MISCOMPARE_PURGE;
 	if (!libxfs_init(&x)) {

Re: [PATCH 2/2] xfs_db: use directio for device access

[Christoph Hellwig]

On Mon, Sep 25, 2023 at 02:59:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> XFS and tools (mkfs, copy, repair) don't generally rely on the block
> device page cache, preferring instead to use directio.  For whatever
> reason, the debugger was never made to do this, but let's do that now.
> 
> This should eliminate the weird fstests failures resulting from
> udev/blkid pinning a cache page while the unmounting filesystem writes
> to the superblock such that xfs_db finds the stale pagecache instead of
> the post-unmount superblock.

After some debugging I found out that this breaks a bunch of tests
(at least xfs/002 xfs/070 xfs/424 in the quick group) on 4k device
because xfs_db tries some unaligned reads.

For xfs/002 that is the libxfs_buf_read in __set_cur, when setting the
type to data, but I haven't looked at the other test in detail.

Should I look into finding all these assumptions in xfs_db, or
just make the direct I/O enablement conditional n a 612 byte sector
size?

Re: [PATCH 2/2] xfs_db: use directio for device access

[Darrick J. Wong]

On Wed, Jan 17, 2024 at 10:30:23AM -0800, Christoph Hellwig wrote:
> On Mon, Sep 25, 2023 at 02:59:16PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > XFS and tools (mkfs, copy, repair) don't generally rely on the block
> > device page cache, preferring instead to use directio.  For whatever
> > reason, the debugger was never made to do this, but let's do that now.
> > 
> > This should eliminate the weird fstests failures resulting from
> > udev/blkid pinning a cache page while the unmounting filesystem writes
> > to the superblock such that xfs_db finds the stale pagecache instead of
> > the post-unmount superblock.
> 
> After some debugging I found out that this breaks a bunch of tests
> (at least xfs/002 xfs/070 xfs/424 in the quick group) on 4k device
> because xfs_db tries some unaligned reads.
> 
> For xfs/002 that is the libxfs_buf_read in __set_cur, when setting the
> type to data, but I haven't looked at the other test in detail.

Hmm.  Perhaps the userspace buftarg setup should go find the physical
sector size of the device?  That "bb_count = 1" in set_iocur_type looks
a bit smelly.

> Should I look into finding all these assumptions in xfs_db, or
> just make the direct I/O enablement conditional n a 612 byte sector
> size?

Let me go run a lbasize=4k fstests run overnight and see what happens.
IIRC zorro told me last year that it wasn't pretty.

--D

Re: [PATCH 2/2] xfs_db: use directio for device access

[Christoph Hellwig]

On Wed, Jan 17, 2024 at 05:32:50PM -0800, Darrick J. Wong wrote:
> > 
> > For xfs/002 that is the libxfs_buf_read in __set_cur, when setting the
> > type to data, but I haven't looked at the other test in detail.
> 
> Hmm.  Perhaps the userspace buftarg setup should go find the physical
> sector size of the device?  That "bb_count = 1" in set_iocur_type looks
> a bit smelly.

Yes, that should fix this particular issue.

> > Should I look into finding all these assumptions in xfs_db, or
> > just make the direct I/O enablement conditional n a 612 byte sector
> > size?
> 
> Let me go run a lbasize=4k fstests run overnight and see what happens.
> IIRC zorro told me last year that it wasn't pretty.

There's a few failures, but I've been slowly trying to fix this.  The
libxfs/mkfs log sector size detection series in one part of that,
and this:

https://lore.kernel.org/linux-block/20240117175901.871796-1-hch@lst.de/T/#u

is another

Re: [PATCH 2/2] xfs_db: use directio for device access

[Darrick J. Wong]

On Wed, Jan 17, 2024 at 08:21:25PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 17, 2024 at 05:32:50PM -0800, Darrick J. Wong wrote:
> > > 
> > > For xfs/002 that is the libxfs_buf_read in __set_cur, when setting the
> > > type to data, but I haven't looked at the other test in detail.
> > 
> > Hmm.  Perhaps the userspace buftarg setup should go find the physical
> > sector size of the device?  That "bb_count = 1" in set_iocur_type looks
> > a bit smelly.
> 
> Yes, that should fix this particular issue.
> 
> > > Should I look into finding all these assumptions in xfs_db, or
> > > just make the direct I/O enablement conditional n a 612 byte sector
> > > size?
> > 
> > Let me go run a lbasize=4k fstests run overnight and see what happens.
> > IIRC zorro told me last year that it wasn't pretty.
> 
> There's a few failures, but I've been slowly trying to fix this.  The
> libxfs/mkfs log sector size detection series in one part of that,
> and this:
> 
> https://lore.kernel.org/linux-block/20240117175901.871796-1-hch@lst.de/T/#u

Hmm well I didn't manage to add your loop device patch before I sent
this out last night, but here's the fstest results:

https://djwong.org/fstests/output/.67c2f90f0a1bb329a1b895c50285b0d23c1bd2bb44b7839f3543f82281665db1/.4a10533d4dd2085d3f996649e0886284f557617c94e604189448672e6009b9e8/

Looks like there were a lot of weird problems.  OFC now the second ice
storm has started and the lights are flickering so that might be all
from me for now.

--D

> is another
> 

Re: [PATCHSET 0/2] xfs_db: use directio for filesystem access

[Carlos Maiolino]

On Mon, Sep 25, 2023 at 02:59:04PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> xfs_db doesn't even try to use directio to access block devices.  This
> is rather strange, since mkfs, copy, and repair use it.  Modify xfs_db
> to set LIBXFS_DIRECT so that we don't have to deal with the bdev
> pagecache being out of sync with what the kernel might have written
> directly to the block device.
> 
> While we're at it, if we're using directio to the block device, don't
> fail if we can't set the bufferhead size to the sectorsize, because
> we're not using the pagecache anyway.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> This has been running on the djcloud for months with no problems.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D

The series looks good, thanks!

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Carlos

> 
> xfsprogs git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=db-use-directio
> ---
>  db/init.c     |    1 +
>  libxfs/init.c |    8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
>