| Message ID | 3b4b9f091519d2b2085888d296888179da3bdb73.1733902742.git.ojaswin@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs_io: enable extsize and stat -v support for ext4 | expand |
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 > >
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.
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 > > > >
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 >
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.
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 >
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 > >
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; }
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(-)