Re: [PATCH 1/2] xfs_io: implement 'inode' command V4

[Carlos Maiolino <cmaiolino@redhat.com>]

Hi Brian,

> 
> Well, the code looks Ok to me but the design still seems overdone IMO. I
> have no major objection if this goes in as is (with one exception noted
> below), but IMO we should take the approach somewhat discussed in the v3
> review.

First of all, thanks for reviewing the patch again, I took some time to reply
because I was waiting for any other comments and had a few other things to do.

> 
> In particular, define an actual default behavior for the command to
> return the largest inode number (or return 1/0 for "ability to mount w/
> inode32" as Dave suggested). For example, kill both of the -l and -s
> flags and just return 1/0 by default. Define a single verbose (-v) flag
> to print the combined inode number and size. This mode can be
> implemented as the body of the inode_f() function. If -n is specified,
> basically do what the current version does. Just my .02.
> 

I agree with you here and tbh, I don't really think a -v flag is really needed,
although it can certainly facilitate the usage of the xfs_io -c "inode".
Checking for the size of the inode returned against UINT32_MAX is not that hard
anyway, but I think keeping a very simple return value for the command might be
the best approach.

I'm going to re-write it and send a V5 today including a review of your comment
below.

thanks again for the review

> >  io/open.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 151 insertions(+)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index ac5a5e0..2fc8aab 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> ...
> > +	if (ret_lsize || ret_largest) {
> > +
> > +		bulkreq.lastip = &last;
> > +		bulkreq.icount = 1024; /* User-defined maybe!? */
> > +		bulkreq.ubuffer = &igroup;
> > +		bulkreq.ocount = &count;
> > +
> > +		for (;;) {
> > +
> > +			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
> > +					&bulkreq)) {
> > +				perror("XFS_IOC_FSINUMBERS");
> > +				exitcode = 1;
> > +				return 0;
> > +			}
> > +
> > +			if (count == 0)
> > +				break;
> > +
> > +			lastgrp = count;
> > +		}
> > +
> > +		lastgrp--;
> > +		igrp_rec = igroup[lastgrp];
> 
> IIRC the point of igrp_rec was to save off the last record so it could
> be used directly after the loop without need for the count (because it
> could be 0). Here we use a separate lastgrp count to protect against the
> 0 case, yet still do the record copy after the loop... what's the point
> of that?
> 
> Brian
> 
> > +		lastino = igrp_rec.xi_startino +
> > +			  xfs_highbit64(igrp_rec.xi_allocmask);
> > +
> > +		if (ret_lsize)
> > +			printf (_("Largest inode size: %d\n"),
> > +				lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> > +		else
> > +			printf(_("Largest inode: %llu\n"), lastino);
> > +
> > +		return 0;
> > +	}
> > +
> > +	return command_usage(&inode_cmd);
> > +}
> > +
> >  void
> >  open_init(void)
> >  {
> > @@ -815,6 +955,16 @@ open_init(void)
> >  		_("get/set preferred extent size (in bytes) for the open file");
> >  	extsize_cmd.help = extsize_help;
> >  
> > +	inode_cmd.name = "inode";
> > +	inode_cmd.cfunc = inode_f;
> > +	inode_cmd.args = _("[-s | -l | -n] [num]");
> > +	inode_cmd.argmin = 1;
> > +	inode_cmd.argmax = 2;
> > +	inode_cmd.flags = CMD_NOMAP_OK;
> > +	inode_cmd.oneline =
> > +		_("Query inode number usage in the filesystem");
> > +	inode_cmd.help = inode_help;
> > +
> >  	add_command(&open_cmd);
> >  	add_command(&stat_cmd);
> >  	add_command(&close_cmd);
> > @@ -822,4 +972,5 @@ open_init(void)
> >  	add_command(&chproj_cmd);
> >  	add_command(&lsproj_cmd);
> >  	add_command(&extsize_cmd);
> > +	add_command(&inode_cmd);
> >  }
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > 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