Re: [PATCH] xfs_io: Implement inodes64 command

[Carlos Maiolino <cmaiolino@redhat.com>]

On Tue, Sep 22, 2015 at 08:08:32AM +1000, Dave Chinner wrote:
> On Mon, Sep 21, 2015 at 10:56:08AM +0200, Carlos Maiolino wrote:
> > Implement an easy way to check a filesystem for the existence of 64bit inodes.
> > 
> > Based on XFS_IOC_FSINUMBERS, and starting with a lastip of XFS_MAXINUMBER_32, it
> > returns a string saying if there is or there isn't 64bit inodes in the
> > filesystem.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  io/open.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index ac5a5e0..b25b09d 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> > @@ -20,6 +20,7 @@
> >  #include "input.h"
> >  #include "init.h"
> >  #include "io.h"
> > +#include "libxfs.h"
> >  
> >  #ifndef __O_TMPFILE
> >  #if defined __alpha__
> > @@ -44,6 +45,7 @@ static cmdinfo_t statfs_cmd;
> >  static cmdinfo_t chproj_cmd;
> >  static cmdinfo_t lsproj_cmd;
> >  static cmdinfo_t extsize_cmd;
> > +static cmdinfo_t inodes64_cmd;
> >  static prid_t prid;
> >  static long extsize;
> >  
> > @@ -750,6 +752,36 @@ statfs_f(
> >  	return 0;
> >  }
> >  
> > +static int
> > +inodes64_f(
> > +	  int			argc,
> > +	  char			**argv)
> > +{
> > +	int			i;
> > +	__s32			count;
> > +	__u64			last = XFS_MAXINUMBER_32;
> > +	xfs_inogrp_t		igroup;
> > +	xfs_fsop_bulkreq_t	bulkreq;
> 
> Please don't use typedefs.
> 
Ok.

> > +
> > +	bulkreq.lastip = &last;
> > +	bulkreq.icount = 1;
> > +	bulkreq.ubuffer = &igroup;
> > +	bulkreq.ocount = &count;
> > +
> > +	if (!xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> > +		if (count > 0) {
> > +			printf("Filesystem does have 64bit inodes\n");
> > +			return 0;
> > +		} else {
> > +			printf("Filesystem does not have 64bit inodes\n");
> > +			return 0;
> > +		}
> > +	}
> > +	perror("xfsctl(XFS_IOC_FSINUMBERS)");
> 
> I'd do this the other way around:
> 
> 	if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq)) {
> 		perror("XFS_IOC_FSINUMBERS");
> 		exitcode = 1;
> 		return 0;
> 	}
> 
> 	if (count > 0)
> 		printf("Filesystem does have 64bit inodes\n");
> 	else
> 		printf("Filesystem does not have 64bit inodes\n");
> 	return 0;
> 
> is sufficient, xfsctl is a one line wrapper around ioctl().
>

Ok, I'll change this, but, just a matter of curiosity, what are the technical
reasons to write the conditional this way, instead of the way I wrote first?
Just to avoid entering the conditional?

 
> > @@ -815,6 +847,12 @@ open_init(void)
> >  		_("get/set preferred extent size (in bytes) for the open file");
> >  	extsize_cmd.help = extsize_help;
> >  
> > +	inodes64_cmd.name = "inodes64";
> > +	inodes64_cmd.cfunc = inodes64_f;
> > +	inodes64_cmd.flags = CMD_NOMAP_OK;
> > +	inodes64_cmd.oneline =
> > +		_("Checks if filesyste contais 64bit inodes");
>                                     ^^^     ^^ 64 bit
> 
> "inodes64" could be improved as a command name. e.g:
> 
> oneline = _("Query inode number usage in the filesystem")
> 
> And could do a little more to help users work out what the
> problematic inode is. e.g:
> 
> Long help:
> 
> Inode_numbers [-s|-l] [num]
> 
> 	[-s]	returns the physical size of the largest inode
> 		number in the filesystem (32 bit of 64 bit)
> 	[-l]	returns the largest inode number in the filesystem
> 	[-n]	returns the next inode number after [num]
> 	[num]	returns whether the inode number exists
> 
> i.e. if you want 32 bit inodes in the fs, you can use [-n num] to
> iterate all the inode numbers above 32 bits in size...
> 

Ok, np

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs