[RFC 0/3] xfs_io: enable extsize and stat -v support for ext4

[RFC 0/3] xfs_io: enable extsize and stat -v support for ext4

[Ojaswin Mujoo]

With ext4 extsize hints support being worked on, enable extsize 
command to be run on FSes other than xfs so ext4 can utilize it.
Also extend stat -v to perform FS_IOC_FSGETXATTR ioctl on ext4.

No funtional changes are intended for XFS.

Ojaswin Mujoo (3):
  include/linux.h: Factor out generic platform_test_fs_fd() helper
  xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details
  xfs_io: add extsize command support

 include/linux.h | 25 +++++++++++++++++--------
 io/open.c       |  2 +-
 io/stat.c       | 38 +++++++++++++++++++++-----------------
 3 files changed, 39 insertions(+), 26 deletions(-)

-- 
2.43.5

[RFC 1/3] include/linux.h: Factor out generic platform_test_fs_fd() helper

[Ojaswin Mujoo]

Factor our the generic code to detect the FS type out of
platform_test_fs_fd(). This can then be used to detect different file
systems types based on magic number.

Also, add a helper to detect if the fd is from an ext4 filesystem.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 include/linux.h | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/linux.h b/include/linux.h
index e9eb7bfb26a1..52c64014c57f 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -43,13 +43,7 @@ static __inline__ int xfsctl(const char *path, int fd, int cmd, void *p)
 	return ioctl(fd, cmd, p);
 }
 
-/*
- * platform_test_xfs_*() implies that xfsctl will succeed on the file;
- * on Linux, at least, special files don't get xfs file ops,
- * so return 0 for those
- */
-
-static __inline__ int platform_test_xfs_fd(int fd)
+static __inline__ int platform_test_fs_fd(int fd, long type)
 {
 	struct statfs statfsbuf;
 	struct stat statbuf;
@@ -60,7 +54,22 @@ static __inline__ int platform_test_xfs_fd(int fd)
 		return 0;
 	if (!S_ISREG(statbuf.st_mode) && !S_ISDIR(statbuf.st_mode))
 		return 0;
-	return (statfsbuf.f_type == 0x58465342);	/* XFSB */
+	return (statfsbuf.f_type == type);
+}
+
+/*
+ * platform_test_xfs_*() implies that xfsctl will succeed on the file;
+ * on Linux, at least, special files don't get xfs file ops,
+ * so return 0 for those
+ */
+static __inline__ int platform_test_xfs_fd(int fd)
+{
+	return platform_test_fs_fd(fd, 0x58465342); /* XFSB */
+}
+
+static __inline__ int platform_test_ext4_fd(int fd)
+{
+	return platform_test_fs_fd(fd, 0xef53); /* EXT4 magic number */
 }
 
 static __inline__ int platform_test_xfs_path(const char *path)
-- 
2.43.5

Re: [RFC 1/3] include/linux.h: Factor out generic platform_test_fs_fd() helper

[Darrick J. Wong]

On Wed, Dec 11, 2024 at 01:24:02PM +0530, Ojaswin Mujoo wrote:
> Factor our the generic code to detect the FS type out of
> platform_test_fs_fd(). This can then be used to detect different file
> systems types based on magic number.
> 
> Also, add a helper to detect if the fd is from an ext4 filesystem.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  include/linux.h | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux.h b/include/linux.h
> index e9eb7bfb26a1..52c64014c57f 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -43,13 +43,7 @@ static __inline__ int xfsctl(const char *path, int fd, int cmd, void *p)
>  	return ioctl(fd, cmd, p);
>  }
>  
> -/*
> - * platform_test_xfs_*() implies that xfsctl will succeed on the file;
> - * on Linux, at least, special files don't get xfs file ops,
> - * so return 0 for those
> - */
> -
> -static __inline__ int platform_test_xfs_fd(int fd)
> +static __inline__ int platform_test_fs_fd(int fd, long type)
>  {
>  	struct statfs statfsbuf;
>  	struct stat statbuf;
> @@ -60,7 +54,22 @@ static __inline__ int platform_test_xfs_fd(int fd)
>  		return 0;
>  	if (!S_ISREG(statbuf.st_mode) && !S_ISDIR(statbuf.st_mode))
>  		return 0;
> -	return (statfsbuf.f_type == 0x58465342);	/* XFSB */
> +	return (statfsbuf.f_type == type);
> +}
> +
> +/*
> + * platform_test_xfs_*() implies that xfsctl will succeed on the file;
> + * on Linux, at least, special files don't get xfs file ops,
> + * so return 0 for those
> + */
> +static __inline__ int platform_test_xfs_fd(int fd)
> +{
> +	return platform_test_fs_fd(fd, 0x58465342); /* XFSB */
> +}
> +
> +static __inline__ int platform_test_ext4_fd(int fd)
> +{
> +	return platform_test_fs_fd(fd, 0xef53); /* EXT4 magic number */

Should this be pulling EXT4_SUPER_MAGIC from linux/magic.h?

--D

>  }
>  
>  static __inline__ int platform_test_xfs_path(const char *path)
> -- 
> 2.43.5
> 
> 

Re: [RFC 1/3] include/linux.h: Factor out generic platform_test_fs_fd() helper

[Ojaswin Mujoo]

On Wed, Dec 11, 2024 at 10:09:02AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 11, 2024 at 01:24:02PM +0530, Ojaswin Mujoo wrote:
> > Factor our the generic code to detect the FS type out of
> > platform_test_fs_fd(). This can then be used to detect different file
> > systems types based on magic number.
> > 
> > Also, add a helper to detect if the fd is from an ext4 filesystem.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  include/linux.h | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux.h b/include/linux.h
> > index e9eb7bfb26a1..52c64014c57f 100644
> > --- a/include/linux.h
> > +++ b/include/linux.h
> > @@ -43,13 +43,7 @@ static __inline__ int xfsctl(const char *path, int fd, int cmd, void *p)
> >  	return ioctl(fd, cmd, p);
> >  }
> >  
> > -/*
> > - * platform_test_xfs_*() implies that xfsctl will succeed on the file;
> > - * on Linux, at least, special files don't get xfs file ops,
> > - * so return 0 for those
> > - */
> > -
> > -static __inline__ int platform_test_xfs_fd(int fd)
> > +static __inline__ int platform_test_fs_fd(int fd, long type)
> >  {
> >  	struct statfs statfsbuf;
> >  	struct stat statbuf;
> > @@ -60,7 +54,22 @@ static __inline__ int platform_test_xfs_fd(int fd)
> >  		return 0;
> >  	if (!S_ISREG(statbuf.st_mode) && !S_ISDIR(statbuf.st_mode))
> >  		return 0;
> > -	return (statfsbuf.f_type == 0x58465342);	/* XFSB */
> > +	return (statfsbuf.f_type == type);
> > +}
> > +
> > +/*
> > + * platform_test_xfs_*() implies that xfsctl will succeed on the file;
> > + * on Linux, at least, special files don't get xfs file ops,
> > + * so return 0 for those
> > + */
> > +static __inline__ int platform_test_xfs_fd(int fd)
> > +{
> > +	return platform_test_fs_fd(fd, 0x58465342); /* XFSB */
> > +}
> > +
> > +static __inline__ int platform_test_ext4_fd(int fd)
> > +{
> > +	return platform_test_fs_fd(fd, 0xef53); /* EXT4 magic number */
> 
> Should this be pulling EXT4_SUPER_MAGIC from linux/magic.h?

Oh right, thanks for pointing out. I'll use that for the magic numbers.

Thanks,
ojaswin

> 
> --D
> 
> >  }
> >  
> >  static __inline__ int platform_test_xfs_path(const char *path)
> > -- 
> > 2.43.5
> > 
> > 

Re: [RFC 1/3] include/linux.h: Factor out generic platform_test_fs_fd() helper

[Christoph Hellwig]

On Wed, Dec 11, 2024 at 10:09:02AM -0800, Darrick J. Wong wrote:
> > +
> > +static __inline__ int platform_test_ext4_fd(int fd)
> > +{
> > +	return platform_test_fs_fd(fd, 0xef53); /* EXT4 magic number */
> 
> Should this be pulling EXT4_SUPER_MAGIC from linux/magic.h?

I think we can just drop adding platform_test_ext4_fd with the
suggested patches later in the series?  Having ext4-specific code
in xfsprogs would seem pretty odd in xfsprogs anyway over our previous
categories of xfs only or generic.

Re: [RFC 1/3] include/linux.h: Factor out generic platform_test_fs_fd() helper

[Ojaswin Mujoo]

On Thu, Dec 12, 2024 at 09:57:01PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 11, 2024 at 10:09:02AM -0800, Darrick J. Wong wrote:
> > > +
> > > +static __inline__ int platform_test_ext4_fd(int fd)
> > > +{
> > > +	return platform_test_fs_fd(fd, 0xef53); /* EXT4 magic number */
> > 
> > Should this be pulling EXT4_SUPER_MAGIC from linux/magic.h?
> 
> I think we can just drop adding platform_test_ext4_fd with the
> suggested patches later in the series?  Having ext4-specific code
> in xfsprogs would seem pretty odd in xfsprogs anyway over our previous
> categories of xfs only or generic.
> 

Oh right, xfs_io is so widely used across FSes that I missed to consider it 
might not be okay to add special code specific to ext4 :)

Anyways, I agree we don't need to separately handle ext4 anymore.  I'll
drop this patch. (Maybe still get XFS_SUPER_MAGIC from linux/magic.h as
that seems like the right thing to do)

Regards,
ojaswin

[RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Ojaswin Mujoo]

Currently with stat we only show FS_IOC_FSGETXATTR details
if the filesystem is XFS. With extsize support also coming
to ext4 make sure to show these details when -c "stat" or "statx"
is used.

No functional changes for filesystems other than ext4.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 io/stat.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/io/stat.c b/io/stat.c
index 326f2822e276..d06c2186cde4 100644
--- a/io/stat.c
+++ b/io/stat.c
@@ -97,14 +97,14 @@ print_file_info(void)
 		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
 }
 
-static void
-print_xfs_info(int verbose)
+static void print_extended_info(int verbose)
 {
-	struct dioattr	dio;
-	struct fsxattr	fsx, fsxa;
+	struct dioattr dio;
+	struct fsxattr fsx, fsxa;
+	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
 
-	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
-	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
+	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
+		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
 		perror("FS_IOC_FSGETXATTR");
 	} else {
 		printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
@@ -113,14 +113,18 @@ print_xfs_info(int verbose)
 		printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
 		printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
 		printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
-		printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
+		if (is_xfs_fd)
+			printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
 	}
-	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
-		perror("XFS_IOC_DIOINFO");
-	} else {
-		printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
-		printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
-		printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
+
+	if (is_xfs_fd) {
+		if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
+			perror("XFS_IOC_DIOINFO");
+		} else {
+			printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
+			printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
+			printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
+		}
 	}
 }
 
@@ -167,10 +171,10 @@ stat_f(
 		printf(_("stat.ctime = %s"), ctime(&st.st_ctime));
 	}
 
-	if (file->flags & IO_FOREIGN)
+	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
 		return 0;
 
-	print_xfs_info(verbose);
+	print_extended_info(verbose);
 
 	return 0;
 }
@@ -440,10 +444,10 @@ statx_f(
 				ctime((time_t *)&stx.stx_btime.tv_sec));
 	}
 
-	if (file->flags & IO_FOREIGN)
+	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
 		return 0;
 
-	print_xfs_info(verbose);
+	print_extended_info(verbose);
 
 	return 0;
 }
-- 
2.43.5

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Darrick J. Wong]

On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> Currently with stat we only show FS_IOC_FSGETXATTR details
> if the filesystem is XFS. With extsize support also coming
> to ext4 make sure to show these details when -c "stat" or "statx"
> is used.
> 
> No functional changes for filesystems other than ext4.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> ---
>  io/stat.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/io/stat.c b/io/stat.c
> index 326f2822e276..d06c2186cde4 100644
> --- a/io/stat.c
> +++ b/io/stat.c
> @@ -97,14 +97,14 @@ print_file_info(void)
>  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
>  }
>  
> -static void
> -print_xfs_info(int verbose)
> +static void print_extended_info(int verbose)
>  {
> -	struct dioattr	dio;
> -	struct fsxattr	fsx, fsxa;
> +	struct dioattr dio;
> +	struct fsxattr fsx, fsxa;
> +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
>  
> -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {

Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
print whatever is returned, no matter what filesystem we think is
feeding us information?

e.g.

	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
		if (is_xfs_fd || (errno != EOPNOTSUPP &&
				  errno != ENOTTY))
			perror("FS_IOC_GETXATTR");
	} else {
		printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
		...
	}

	if (ioctl(file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
		if (is_xfs_fd || (errno != EOPNOTSUPP &&
				  errno != ENOTTY))
			perror("XFS_IOC_FSGETXATTRA");
	} else {
		printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
	}

That way we don't have to specialcase platform_test_*_fd() for every
other filesystem that might want to return real fsxattr results?
Same idea for DIOINFO.

--D

>  		perror("FS_IOC_FSGETXATTR");
>  	} else {
>  		printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> @@ -113,14 +113,18 @@ print_xfs_info(int verbose)
>  		printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
>  		printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
>  		printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
> -		printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> +		if (is_xfs_fd)
> +			printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
>  	}
> -	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> -		perror("XFS_IOC_DIOINFO");
> -	} else {
> -		printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> -		printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> -		printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> +
> +	if (is_xfs_fd) {
> +		if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> +			perror("XFS_IOC_DIOINFO");
> +		} else {
> +			printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> +			printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> +			printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> +		}
>  	}
>  }
>  
> @@ -167,10 +171,10 @@ stat_f(
>  		printf(_("stat.ctime = %s"), ctime(&st.st_ctime));
>  	}
>  
> -	if (file->flags & IO_FOREIGN)
> +	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
>  		return 0;
>  
> -	print_xfs_info(verbose);
> +	print_extended_info(verbose);
>  
>  	return 0;
>  }
> @@ -440,10 +444,10 @@ statx_f(
>  				ctime((time_t *)&stx.stx_btime.tv_sec));
>  	}
>  
> -	if (file->flags & IO_FOREIGN)
> +	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
>  		return 0;
>  
> -	print_xfs_info(verbose);
> +	print_extended_info(verbose);
>  
>  	return 0;
>  }
> -- 
> 2.43.5
> 
> 

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Dave Chinner]

On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > Currently with stat we only show FS_IOC_FSGETXATTR details
> > if the filesystem is XFS. With extsize support also coming
> > to ext4 make sure to show these details when -c "stat" or "statx"
> > is used.
> > 
> > No functional changes for filesystems other than ext4.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  io/stat.c | 38 +++++++++++++++++++++-----------------
> >  1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/io/stat.c b/io/stat.c
> > index 326f2822e276..d06c2186cde4 100644
> > --- a/io/stat.c
> > +++ b/io/stat.c
> > @@ -97,14 +97,14 @@ print_file_info(void)
> >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> >  }
> >  
> > -static void
> > -print_xfs_info(int verbose)
> > +static void print_extended_info(int verbose)
> >  {
> > -	struct dioattr	dio;
> > -	struct fsxattr	fsx, fsxa;
> > +	struct dioattr dio;
> > +	struct fsxattr fsx, fsxa;
> > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> >  
> > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> 
> Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> print whatever is returned, no matter what filesystem we think is
> feeding us information?

Yes, please. FS_IOC_FSGETXATTR has been generic functionality for
some time, we should treat it the same way for all filesystems.

> e.g.
> 
> 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> 				  errno != ENOTTY))
> 			perror("FS_IOC_GETXATTR");

Why do we even need "is_xfs_fd" there? XFS will never give a
EOPNOTSUPP or ENOTTY error to this or the FS_IOC_GETXATTRA ioctl...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Darrick J. Wong]

On Thu, Dec 12, 2024 at 09:33:29AM +1100, Dave Chinner wrote:
> On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> > On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > > Currently with stat we only show FS_IOC_FSGETXATTR details
> > > if the filesystem is XFS. With extsize support also coming
> > > to ext4 make sure to show these details when -c "stat" or "statx"
> > > is used.
> > > 
> > > No functional changes for filesystems other than ext4.
> > > 
> > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > ---
> > >  io/stat.c | 38 +++++++++++++++++++++-----------------
> > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/io/stat.c b/io/stat.c
> > > index 326f2822e276..d06c2186cde4 100644
> > > --- a/io/stat.c
> > > +++ b/io/stat.c
> > > @@ -97,14 +97,14 @@ print_file_info(void)
> > >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> > >  }
> > >  
> > > -static void
> > > -print_xfs_info(int verbose)
> > > +static void print_extended_info(int verbose)
> > >  {
> > > -	struct dioattr	dio;
> > > -	struct fsxattr	fsx, fsxa;
> > > +	struct dioattr dio;
> > > +	struct fsxattr fsx, fsxa;
> > > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> > >  
> > > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> > 
> > Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> > print whatever is returned, no matter what filesystem we think is
> > feeding us information?
> 
> Yes, please. FS_IOC_FSGETXATTR has been generic functionality for
> some time, we should treat it the same way for all filesystems.
> 
> > e.g.
> > 
> > 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> > 				  errno != ENOTTY))
> > 			perror("FS_IOC_GETXATTR");
> 
> Why do we even need "is_xfs_fd" there? XFS will never give a
> EOPNOTSUPP or ENOTTY error to this or the FS_IOC_GETXATTRA ioctl...

Yeah, in hindsight I don't think it's needed for FS_IOC_FSGETXATTR, but
it's definitely nice for XFS_IOC_FSGETXATTRA (which is not implemented
outside xfs) so that you don't get unnecessary error messages on ext4.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Dave Chinner]

On Thu, Dec 12, 2024 at 08:19:19AM -0800, Darrick J. Wong wrote:
> On Thu, Dec 12, 2024 at 09:33:29AM +1100, Dave Chinner wrote:
> > On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> > > On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > > > Currently with stat we only show FS_IOC_FSGETXATTR details
> > > > if the filesystem is XFS. With extsize support also coming
> > > > to ext4 make sure to show these details when -c "stat" or "statx"
> > > > is used.
> > > > 
> > > > No functional changes for filesystems other than ext4.
> > > > 
> > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > ---
> > > >  io/stat.c | 38 +++++++++++++++++++++-----------------
> > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/io/stat.c b/io/stat.c
> > > > index 326f2822e276..d06c2186cde4 100644
> > > > --- a/io/stat.c
> > > > +++ b/io/stat.c
> > > > @@ -97,14 +97,14 @@ print_file_info(void)
> > > >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> > > >  }
> > > >  
> > > > -static void
> > > > -print_xfs_info(int verbose)
> > > > +static void print_extended_info(int verbose)
> > > >  {
> > > > -	struct dioattr	dio;
> > > > -	struct fsxattr	fsx, fsxa;
> > > > +	struct dioattr dio;
> > > > +	struct fsxattr fsx, fsxa;
> > > > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> > > >  
> > > > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > > > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> > > 
> > > Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> > > print whatever is returned, no matter what filesystem we think is
> > > feeding us information?
> > 
> > Yes, please. FS_IOC_FSGETXATTR has been generic functionality for
> > some time, we should treat it the same way for all filesystems.
> > 
> > > e.g.
> > > 
> > > 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > > 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> > > 				  errno != ENOTTY))
> > > 			perror("FS_IOC_GETXATTR");
> > 
> > Why do we even need "is_xfs_fd" there? XFS will never give a
> > EOPNOTSUPP or ENOTTY error to this or the FS_IOC_GETXATTRA ioctl...
> 
> Yeah, in hindsight I don't think it's needed for FS_IOC_FSGETXATTR, but

*nod*

> it's definitely nice for XFS_IOC_FSGETXATTRA (which is not implemented
> outside xfs) so that you don't get unnecessary error messages on ext4.

I don't think we even need it for FS_IOC_GETXATTRA - if the
filesystem does not support that ioctl, we don't print the fields,
nor do we output an error.

After all, this "extended info" and it's only ever been printed
for XFS, so we can define whatever semantics we want for foreign
filesystem output right now. As long as XFS always prints the same
info as it always has (i.e. all of it), we can do whatever we want
with the foreign filesystem stuff.

Keep in mind that we don't need platform tests for XFS files - that
has already been done when the file was opened and the state stored
in file->flags via the IO_FOREIGN flag. We already use that in the
stat_f() to determine whether we print the "xfs info" or not.

IOWs, I think all we need to do is  move where we check the
IO_FOREIGN flag. i.e.:

print_extented_info(file)
{
	struct dioattr  dio = {};
        struct fsxattr  fsx = {}, fsxa = {};

	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
		perror("FS_IOC_GETXATTR");
		exitcode = 1;
		return;
	}

	printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
	printxattr(fsx.fsx_xflags, verbose, 0, file->name, 1, 1);
	printf(_("fsxattr.projid = %u\n"), fsx.fsx_projid);
	printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
	printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
	printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);

	/* Only XFS supports FS_IOC_FSGETXATTRA and XFS_IOC_DIOINFO */
	if (file->flags & IO_FOREIGN)
		return;

	if (ioctl(file->fd, FS_IOC_FSGETXATTRA, &fsxa)) < 0) {
		perror("FS_IOC_GETXATTRA");
		exitcode = 1;
		return;
	}
	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
		perror("XFS_IOC_DIOINFO");
		exitcode = 1;
		return;
	}

	printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
	printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
	printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
	printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
}

Thoughts?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Darrick J. Wong]

On Fri, Dec 13, 2024 at 07:44:01AM +1100, Dave Chinner wrote:
> On Thu, Dec 12, 2024 at 08:19:19AM -0800, Darrick J. Wong wrote:
> > On Thu, Dec 12, 2024 at 09:33:29AM +1100, Dave Chinner wrote:
> > > On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> > > > On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > > > > Currently with stat we only show FS_IOC_FSGETXATTR details
> > > > > if the filesystem is XFS. With extsize support also coming
> > > > > to ext4 make sure to show these details when -c "stat" or "statx"
> > > > > is used.
> > > > > 
> > > > > No functional changes for filesystems other than ext4.
> > > > > 
> > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > > ---
> > > > >  io/stat.c | 38 +++++++++++++++++++++-----------------
> > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > > 
> > > > > diff --git a/io/stat.c b/io/stat.c
> > > > > index 326f2822e276..d06c2186cde4 100644
> > > > > --- a/io/stat.c
> > > > > +++ b/io/stat.c
> > > > > @@ -97,14 +97,14 @@ print_file_info(void)
> > > > >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> > > > >  }
> > > > >  
> > > > > -static void
> > > > > -print_xfs_info(int verbose)
> > > > > +static void print_extended_info(int verbose)
> > > > >  {
> > > > > -	struct dioattr	dio;
> > > > > -	struct fsxattr	fsx, fsxa;
> > > > > +	struct dioattr dio;
> > > > > +	struct fsxattr fsx, fsxa;
> > > > > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> > > > >  
> > > > > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > > > > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> > > > 
> > > > Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> > > > print whatever is returned, no matter what filesystem we think is
> > > > feeding us information?
> > > 
> > > Yes, please. FS_IOC_FSGETXATTR has been generic functionality for
> > > some time, we should treat it the same way for all filesystems.
> > > 
> > > > e.g.
> > > > 
> > > > 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > > > 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> > > > 				  errno != ENOTTY))
> > > > 			perror("FS_IOC_GETXATTR");
> > > 
> > > Why do we even need "is_xfs_fd" there? XFS will never give a
> > > EOPNOTSUPP or ENOTTY error to this or the FS_IOC_GETXATTRA ioctl...
> > 
> > Yeah, in hindsight I don't think it's needed for FS_IOC_FSGETXATTR, but
> 
> *nod*
> 
> > it's definitely nice for XFS_IOC_FSGETXATTRA (which is not implemented
> > outside xfs) so that you don't get unnecessary error messages on ext4.
> 
> I don't think we even need it for FS_IOC_GETXATTRA - if the
> filesystem does not support that ioctl, we don't print the fields,
> nor do we output an error.
> 
> After all, this "extended info" and it's only ever been printed
> for XFS, so we can define whatever semantics we want for foreign
> filesystem output right now. As long as XFS always prints the same
> info as it always has (i.e. all of it), we can do whatever we want
> with the foreign filesystem stuff.
> 
> Keep in mind that we don't need platform tests for XFS files - that
> has already been done when the file was opened and the state stored
> in file->flags via the IO_FOREIGN flag. We already use that in the
> stat_f() to determine whether we print the "xfs info" or not.
> 
> IOWs, I think all we need to do is  move where we check the
> IO_FOREIGN flag. i.e.:
> 
> print_extented_info(file)
> {
> 	struct dioattr  dio = {};
>         struct fsxattr  fsx = {}, fsxa = {};
> 
> 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> 		perror("FS_IOC_GETXATTR");
> 		exitcode = 1;
> 		return;
> 	}
> 
> 	printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> 	printxattr(fsx.fsx_xflags, verbose, 0, file->name, 1, 1);
> 	printf(_("fsxattr.projid = %u\n"), fsx.fsx_projid);
> 	printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
> 	printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
> 	printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
> 
> 	/* Only XFS supports FS_IOC_FSGETXATTRA and XFS_IOC_DIOINFO */
> 	if (file->flags & IO_FOREIGN)
> 		return;
> 
> 	if (ioctl(file->fd, FS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> 		perror("FS_IOC_GETXATTRA");
> 		exitcode = 1;
> 		return;
> 	}
> 	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> 		perror("XFS_IOC_DIOINFO");
> 		exitcode = 1;
> 		return;
> 	}
> 
> 	printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> 	printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> 	printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> 	printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> }
> 
> Thoughts?

Seems fine to me, though I'd print the fsxa before trying to call
DIOINFO.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Ojaswin Mujoo]

On Thu, Dec 12, 2024 at 01:07:58PM -0800, Darrick J. Wong wrote:
> On Fri, Dec 13, 2024 at 07:44:01AM +1100, Dave Chinner wrote:
> > On Thu, Dec 12, 2024 at 08:19:19AM -0800, Darrick J. Wong wrote:
> > > On Thu, Dec 12, 2024 at 09:33:29AM +1100, Dave Chinner wrote:
> > > > On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> > > > > On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > > > > > Currently with stat we only show FS_IOC_FSGETXATTR details
> > > > > > if the filesystem is XFS. With extsize support also coming
> > > > > > to ext4 make sure to show these details when -c "stat" or "statx"
> > > > > > is used.
> > > > > > 
> > > > > > No functional changes for filesystems other than ext4.
> > > > > > 
> > > > > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > > > > > ---
> > > > > >  io/stat.c | 38 +++++++++++++++++++++-----------------
> > > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > diff --git a/io/stat.c b/io/stat.c
> > > > > > index 326f2822e276..d06c2186cde4 100644
> > > > > > --- a/io/stat.c
> > > > > > +++ b/io/stat.c
> > > > > > @@ -97,14 +97,14 @@ print_file_info(void)
> > > > > >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> > > > > >  }
> > > > > >  
> > > > > > -static void
> > > > > > -print_xfs_info(int verbose)
> > > > > > +static void print_extended_info(int verbose)
> > > > > >  {
> > > > > > -	struct dioattr	dio;
> > > > > > -	struct fsxattr	fsx, fsxa;
> > > > > > +	struct dioattr dio;
> > > > > > +	struct fsxattr fsx, fsxa;
> > > > > > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> > > > > >  
> > > > > > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > > > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > > > > > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > > > > > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> > > > > 
> > > > > Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> > > > > print whatever is returned, no matter what filesystem we think is
> > > > > feeding us information?
> > > > 
> > > > Yes, please. FS_IOC_FSGETXATTR has been generic functionality for
> > > > some time, we should treat it the same way for all filesystems.
> > > > 
> > > > > e.g.
> > > > > 
> > > > > 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > > > > 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> > > > > 				  errno != ENOTTY))
> > > > > 			perror("FS_IOC_GETXATTR");
> > > > 
> > > > Why do we even need "is_xfs_fd" there? XFS will never give a
> > > > EOPNOTSUPP or ENOTTY error to this or the FS_IOC_GETXATTRA ioctl...
> > > 
> > > Yeah, in hindsight I don't think it's needed for FS_IOC_FSGETXATTR, but
> > 
> > *nod*
> > 
> > > it's definitely nice for XFS_IOC_FSGETXATTRA (which is not implemented
> > > outside xfs) so that you don't get unnecessary error messages on ext4.
> > 
> > I don't think we even need it for FS_IOC_GETXATTRA - if the
> > filesystem does not support that ioctl, we don't print the fields,
> > nor do we output an error.
> > 
> > After all, this "extended info" and it's only ever been printed
> > for XFS, so we can define whatever semantics we want for foreign
> > filesystem output right now. As long as XFS always prints the same
> > info as it always has (i.e. all of it), we can do whatever we want
> > with the foreign filesystem stuff.
> > 
> > Keep in mind that we don't need platform tests for XFS files - that
> > has already been done when the file was opened and the state stored
> > in file->flags via the IO_FOREIGN flag. We already use that in the
> > stat_f() to determine whether we print the "xfs info" or not.
> > 
> > IOWs, I think all we need to do is  move where we check the
> > IO_FOREIGN flag. i.e.:
> > 
> > print_extented_info(file)
> > {
> > 	struct dioattr  dio = {};
> >         struct fsxattr  fsx = {}, fsxa = {};
> > 
> > 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > 		perror("FS_IOC_GETXATTR");
> > 		exitcode = 1;
> > 		return;
> > 	}
> > 
> > 	printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> > 	printxattr(fsx.fsx_xflags, verbose, 0, file->name, 1, 1);
> > 	printf(_("fsxattr.projid = %u\n"), fsx.fsx_projid);
> > 	printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
> > 	printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
> > 	printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
> > 
> > 	/* Only XFS supports FS_IOC_FSGETXATTRA and XFS_IOC_DIOINFO */
> > 	if (file->flags & IO_FOREIGN)
> > 		return;
> > 
> > 	if (ioctl(file->fd, FS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > 		perror("FS_IOC_GETXATTRA");
> > 		exitcode = 1;
> > 		return;
> > 	}
> > 	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> > 		perror("XFS_IOC_DIOINFO");
> > 		exitcode = 1;
> > 		return;
> > 	}
> > 
> > 	printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> > 	printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> > 	printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> > 	printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> > }
> > 
> > Thoughts?
> 
> Seems fine to me, though I'd print the fsxa before trying to call
> DIOINFO.
> 
> --D

Got it, this makes sense to me as well. I'll do something like this in
v2. Thanks!

Regards,
ojaswin

> 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 

Re: [RFC 2/3] xfs_io: Add ext4 support to show FS_IOC_FSGETXATTR details

[Ojaswin Mujoo]

On Wed, Dec 11, 2024 at 10:17:06AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 11, 2024 at 01:24:03PM +0530, Ojaswin Mujoo wrote:
> > Currently with stat we only show FS_IOC_FSGETXATTR details
> > if the filesystem is XFS. With extsize support also coming
> > to ext4 make sure to show these details when -c "stat" or "statx"
> > is used.
> > 
> > No functional changes for filesystems other than ext4.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > ---
> >  io/stat.c | 38 +++++++++++++++++++++-----------------
> >  1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/io/stat.c b/io/stat.c
> > index 326f2822e276..d06c2186cde4 100644
> > --- a/io/stat.c
> > +++ b/io/stat.c
> > @@ -97,14 +97,14 @@ print_file_info(void)
> >  		file->flags & IO_TMPFILE ? _(",tmpfile") : "");
> >  }
> >  
> > -static void
> > -print_xfs_info(int verbose)
> > +static void print_extended_info(int verbose)
> >  {
> > -	struct dioattr	dio;
> > -	struct fsxattr	fsx, fsxa;
> > +	struct dioattr dio;
> > +	struct fsxattr fsx, fsxa;
> > +	bool is_xfs_fd = platform_test_xfs_fd(file->fd);
> >  
> > -	if ((xfsctl(file->name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > -	    (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> > +	if ((ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0 ||
> > +		(is_xfs_fd && (xfsctl(file->name, file->fd, XFS_IOC_FSGETXATTRA, &fsxa) < 0))) {
> 
> Urgh... perhaps we should call FS_IOC_FSGETXATTR and if it returns zero
> print whatever is returned, no matter what filesystem we think is
> feeding us information?
> 
> e.g.
> 
> 	if (ioctl(file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> 				  errno != ENOTTY))
> 			perror("FS_IOC_GETXATTR");
> 	} else {
> 		printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> 		...
> 	}
> 
> 	if (ioctl(file->fd, XFS_IOC_FSGETXATTRA, &fsxa)) < 0) {
> 		if (is_xfs_fd || (errno != EOPNOTSUPP &&
> 				  errno != ENOTTY))
> 			perror("XFS_IOC_FSGETXATTRA");
> 	} else {
> 		printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> 	}
> 
> That way we don't have to specialcase platform_test_*_fd() for every
> other filesystem that might want to return real fsxattr results?
> Same idea for DIOINFO.

Hi Darrick, thanks for the review.

I agree that this looks like a more modular approach, I'll make the
change. IIUC we basically want to perform the ioctls regardless of the
FS and then handle the error/output accordingly here so that we wont need the 

    if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
            return 0;

line in the stat functions, right?

Also, with the suggested approach the user visible behavior might change
subtly because earlier we used to fail if either FSGETXATTR or
FGGETXATTRA failed but now theres a slim chance that we might print the
output partially. It might not be a very big deal but just thought I'd
point out.

Regards,
ojaswin

> 
> --D
> 
> >  		perror("FS_IOC_FSGETXATTR");
> >  	} else {
> >  		printf(_("fsxattr.xflags = 0x%x "), fsx.fsx_xflags);
> > @@ -113,14 +113,18 @@ print_xfs_info(int verbose)
> >  		printf(_("fsxattr.extsize = %u\n"), fsx.fsx_extsize);
> >  		printf(_("fsxattr.cowextsize = %u\n"), fsx.fsx_cowextsize);
> >  		printf(_("fsxattr.nextents = %u\n"), fsx.fsx_nextents);
> > -		printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> > +		if (is_xfs_fd)
> > +			printf(_("fsxattr.naextents = %u\n"), fsxa.fsx_nextents);
> >  	}
> > -	if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> > -		perror("XFS_IOC_DIOINFO");
> > -	} else {
> > -		printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> > -		printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> > -		printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> > +
> > +	if (is_xfs_fd) {
> > +		if ((xfsctl(file->name, file->fd, XFS_IOC_DIOINFO, &dio)) < 0) {
> > +			perror("XFS_IOC_DIOINFO");
> > +		} else {
> > +			printf(_("dioattr.mem = 0x%x\n"), dio.d_mem);
> > +			printf(_("dioattr.miniosz = %u\n"), dio.d_miniosz);
> > +			printf(_("dioattr.maxiosz = %u\n"), dio.d_maxiosz);
> > +		}
> >  	}
> >  }
> >  
> > @@ -167,10 +171,10 @@ stat_f(
> >  		printf(_("stat.ctime = %s"), ctime(&st.st_ctime));
> >  	}
> >  
> > -	if (file->flags & IO_FOREIGN)
> > +	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
> >  		return 0;
> >  
> > -	print_xfs_info(verbose);
> > +	print_extended_info(verbose);
> >  
> >  	return 0;
> >  }
> > @@ -440,10 +444,10 @@ statx_f(
> >  				ctime((time_t *)&stx.stx_btime.tv_sec));
> >  	}
> >  
> > -	if (file->flags & IO_FOREIGN)
> > +	if (file->flags & IO_FOREIGN && !platform_test_ext4_fd(file->fd))
> >  		return 0;
> >  
> > -	print_xfs_info(verbose);
> > +	print_extended_info(verbose);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.43.5
> > 
> > 

[RFC 3/3] xfs_io: add extsize command support

[Ojaswin Mujoo]

extsize command is currently only supported with XFS filesystem.
Lift this restriction now that ext4 is also supporting extsize hints.

Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
 io/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/open.c b/io/open.c
index a30dd89a1fd5..2582ff9b862e 100644
--- a/io/open.c
+++ b/io/open.c
@@ -997,7 +997,7 @@ open_init(void)
 	extsize_cmd.args = _("[-D | -R] [extsize]");
 	extsize_cmd.argmin = 0;
 	extsize_cmd.argmax = -1;
-	extsize_cmd.flags = CMD_NOMAP_OK;
+	extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
 	extsize_cmd.oneline =
 		_("get/set preferred extent size (in bytes) for the open file");
 	extsize_cmd.help = extsize_help;
-- 
2.43.5

Re: [RFC 3/3] xfs_io: add extsize command support

[Darrick J. Wong]

On Wed, Dec 11, 2024 at 01:24:04PM +0530, Ojaswin Mujoo wrote:
> extsize command is currently only supported with XFS filesystem.
> Lift this restriction now that ext4 is also supporting extsize hints.
> 
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Seems pretty straightforward to me.  Are you planning to add an extsize
option to chattr?

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  io/open.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/io/open.c b/io/open.c
> index a30dd89a1fd5..2582ff9b862e 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -997,7 +997,7 @@ open_init(void)
>  	extsize_cmd.args = _("[-D | -R] [extsize]");
>  	extsize_cmd.argmin = 0;
>  	extsize_cmd.argmax = -1;
> -	extsize_cmd.flags = CMD_NOMAP_OK;
> +	extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
>  	extsize_cmd.oneline =
>  		_("get/set preferred extent size (in bytes) for the open file");
>  	extsize_cmd.help = extsize_help;
> -- 
> 2.43.5
> 
> 

Re: [RFC 3/3] xfs_io: add extsize command support

[Ojaswin Mujoo]

On Wed, Dec 11, 2024 at 10:18:27AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 11, 2024 at 01:24:04PM +0530, Ojaswin Mujoo wrote:
> > extsize command is currently only supported with XFS filesystem.
> > Lift this restriction now that ext4 is also supporting extsize hints.
> > 
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> 
> Seems pretty straightforward to me.  Are you planning to add an extsize
> option to chattr?

Do you mean e2fsprogs? If so, then yes we'll add it there eventually
however for now I only have xfs_io patches since I was working on them 
to make the extsize xfstests work with ext4.

Regards,
ojaswin
> 
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> --D
> 
> > ---
> >  io/open.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index a30dd89a1fd5..2582ff9b862e 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> > @@ -997,7 +997,7 @@ open_init(void)
> >  	extsize_cmd.args = _("[-D | -R] [extsize]");
> >  	extsize_cmd.argmin = 0;
> >  	extsize_cmd.argmax = -1;
> > -	extsize_cmd.flags = CMD_NOMAP_OK;
> > +	extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> >  	extsize_cmd.oneline =
> >  		_("get/set preferred extent size (in bytes) for the open file");
> >  	extsize_cmd.help = extsize_help;
> > -- 
> > 2.43.5
> > 
> > 

Re: [RFC 3/3] xfs_io: add extsize command support

[Darrick J. Wong]

On Thu, Dec 12, 2024 at 05:34:17PM +0530, Ojaswin Mujoo wrote:
> On Wed, Dec 11, 2024 at 10:18:27AM -0800, Darrick J. Wong wrote:
> > On Wed, Dec 11, 2024 at 01:24:04PM +0530, Ojaswin Mujoo wrote:
> > > extsize command is currently only supported with XFS filesystem.
> > > Lift this restriction now that ext4 is also supporting extsize hints.
> > > 
> > > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > 
> > Seems pretty straightforward to me.  Are you planning to add an extsize
> > option to chattr?
> 
> Do you mean e2fsprogs? If so, then yes we'll add it there eventually
> however for now I only have xfs_io patches since I was working on them 
> to make the extsize xfstests work with ext4.

Yep, and good to know (about adding chattr support eventually).

--D

> Regards,
> ojaswin
> > 
> > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> > 
> > --D
> > 
> > > ---
> > >  io/open.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/io/open.c b/io/open.c
> > > index a30dd89a1fd5..2582ff9b862e 100644
> > > --- a/io/open.c
> > > +++ b/io/open.c
> > > @@ -997,7 +997,7 @@ open_init(void)
> > >  	extsize_cmd.args = _("[-D | -R] [extsize]");
> > >  	extsize_cmd.argmin = 0;
> > >  	extsize_cmd.argmax = -1;
> > > -	extsize_cmd.flags = CMD_NOMAP_OK;
> > > +	extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> > >  	extsize_cmd.oneline =
> > >  		_("get/set preferred extent size (in bytes) for the open file");
> > >  	extsize_cmd.help = extsize_help;
> > > -- 
> > > 2.43.5
> > > 
> > > 
>