[PATCH] xfs_io: implement 'inode' command V3

[PATCH] xfs_io: implement 'inode' command V3

[Carlos Maiolino]

Implements a new xfs_io command, named 'inode', which is supposed to be used to
query information about inode's existence and its physical size in the
filesystem.

Currently supporting three arguments:
-s       -- return physical size of the largest inode
-l       -- return the largest inode number allocated and used
-n [num] -- Return the next existing inode after [num], even if [num] is not
            allocated/used
[num]    -- Return if the inode exists or not.

I didn't send the man page patch because I'm sure I'll get some points to
improve, and I'll write the manpage for the next revision.

- Changelog

V3:
	- Merge all 3 patches from the V2 together in a single patch
	- Rework of '-n [num]' and 'num' only arguments algorithm
	- Argument -n now relies on bulkreq.count to check for next inodes, not
	  on bstat.bs_ino anymore.
	- for loop in ret_lsize or ret_largest case, now relies on count being 0
	  to break the loop

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 io/open.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)

diff --git a/io/open.c b/io/open.c
index ac5a5e0..59b5c94 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 inode_cmd;
 static prid_t prid;
 static long extsize;
 
@@ -750,6 +752,154 @@ statfs_f(
 	return 0;
 }
 
+static void
+inode_help(void)
+{
+	printf(_(
+"\n"
+"Query physical information about the inode"
+"\n"
+" -l	-- Returns the largest inode number in the filesystem\n"
+" -s	-- Returns the physical size (in bits) of the\n"
+"	   largest inode number in the filesystem\n"
+" -n	-- Return the next valid inode after [num]"
+"[num]	   Check if the inode [num] exists in the filesystem"
+"\n"));
+}
+
+static int
+inode_f(
+	  int			argc,
+	  char			**argv)
+{
+	__s32			count = 0;
+	__s32			lastgrp = 0;
+	__u64			last = 0;
+	__u64			lastino = 0;
+	__u64			userino = 0;
+	char			*p;
+	int			c;
+	int			ret_lsize = 0;
+	int			ret_largest = 0;
+	int			ret_next = 0;
+	struct xfs_inogrp	igroup[1024], igrp_rec[1024];
+	struct xfs_fsop_bulkreq	bulkreq;
+	struct xfs_bstat	bstat;
+
+
+	while ((c = getopt(argc, argv, "sln:")) != EOF) {
+		switch (c) {
+		case 's':
+			ret_lsize = 1;
+			break;
+		case 'l':
+			ret_largest = 1;
+			break;
+		case 'n':
+			ret_next = 1;
+			userino = strtoull(optarg, &p, 10);
+			break;
+		default:
+			return command_usage(&inode_cmd);
+		}
+	}
+
+	if (((optind < argc) || ret_next) &&
+	     !(ret_lsize || ret_largest)) {
+
+		/* Setup bulkreq for -n or [num] only */
+		bulkreq.lastip = &last;
+		bulkreq.icount = 1;
+		bulkreq.ubuffer = &bstat;
+		bulkreq.ocount = &count;
+
+		if (ret_next) {
+				if ((*p != '\0')) {
+					printf(_("[num] must be a numeric value\n"));
+					exitcode = 1;
+					return 0;
+				}
+			last = userino;
+			if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT,
+					&bulkreq)) {
+				if (errno == EINVAL)
+					printf(_("Invalid or non-existent inode\n"));
+				else
+					perror("XFS_IOC_FSBULKSTAT");
+				exitcode = 1;
+				return 0;
+			}
+
+			if (!count) {
+			printf(_("There are no further inodes in the filesystem\n"));
+			return 0;
+			}
+
+			printf(_("Next inode: %llu\n"), bstat.bs_ino);
+			return 0;
+		} else {
+			userino = strtoull(argv[optind], &p, 10);
+				if ((*p != '\0')) {
+					printf(_("[num] must be a numeric value\n"));
+					exitcode = 1;
+					return 0;
+				}
+			last = userino;
+			if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT_SINGLE,
+					&bulkreq)) {
+				if (errno == EINVAL) {
+					printf(_("Invalid or non-existent inode number\n"));
+				} else
+					perror("XFS_IOC_FSBULKSTAT_SINGLE");
+				exitcode = 1;
+				return 0;
+			}
+			printf(_("Valid inode: %llu\n"), bstat.bs_ino);
+			return 0;
+
+		}
+	}
+
+	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;
+			memcpy(igrp_rec, igroup,
+				sizeof(struct xfs_inogrp) * 1024);
+		}
+
+		lastgrp--;
+		lastino = igrp_rec[lastgrp].xi_startino +
+			  xfs_highbit64(igrp_rec[lastgrp].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 +965,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 +982,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

Re: [PATCH] xfs_io: implement 'inode' command V3

[Brian Foster]

On Mon, Oct 19, 2015 at 02:31:20PM +0200, Carlos Maiolino wrote:
> Implements a new xfs_io command, named 'inode', which is supposed to be used to
> query information about inode's existence and its physical size in the
> filesystem.
> 
> Currently supporting three arguments:
> -s       -- return physical size of the largest inode
> -l       -- return the largest inode number allocated and used
> -n [num] -- Return the next existing inode after [num], even if [num] is not
>             allocated/used
> [num]    -- Return if the inode exists or not.
> 
> I didn't send the man page patch because I'm sure I'll get some points to
> improve, and I'll write the manpage for the next revision.
> 
> - Changelog
> 
> V3:
> 	- Merge all 3 patches from the V2 together in a single patch
> 	- Rework of '-n [num]' and 'num' only arguments algorithm
> 	- Argument -n now relies on bulkreq.count to check for next inodes, not
> 	  on bstat.bs_ino anymore.
> 	- for loop in ret_lsize or ret_largest case, now relies on count being 0
> 	  to break the loop
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  io/open.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 161 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index ac5a5e0..59b5c94 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 inode_cmd;
>  static prid_t prid;
>  static long extsize;
>  
> @@ -750,6 +752,154 @@ statfs_f(
>  	return 0;
>  }
>  
> +static void
> +inode_help(void)
> +{
> +	printf(_(
> +"\n"
> +"Query physical information about the inode"
> +"\n"
> +" -l	-- Returns the largest inode number in the filesystem\n"
> +" -s	-- Returns the physical size (in bits) of the\n"
> +"	   largest inode number in the filesystem\n"
> +" -n	-- Return the next valid inode after [num]"

Missing newline at the end of the above line.

> +"[num]	   Check if the inode [num] exists in the filesystem"

I would word this to say check if the filesystem "is used" in the
filesystem (or "is linked", or something like that) rather than
"exists," simply because this confuses me about whether the inode needs
to be physically allocated (in a chunk) or actually in-use by the fs. A
quick run of the command suggests that the inode must actually be
linked.

> +"\n"));
> +}
> +
> +static int
> +inode_f(
> +	  int			argc,
> +	  char			**argv)
> +{
> +	__s32			count = 0;
> +	__s32			lastgrp = 0;
> +	__u64			last = 0;
> +	__u64			lastino = 0;
> +	__u64			userino = 0;
> +	char			*p;
> +	int			c;
> +	int			ret_lsize = 0;
> +	int			ret_largest = 0;
> +	int			ret_next = 0;
> +	struct xfs_inogrp	igroup[1024], igrp_rec[1024];
> +	struct xfs_fsop_bulkreq	bulkreq;
> +	struct xfs_bstat	bstat;
> +
> +
> +	while ((c = getopt(argc, argv, "sln:")) != EOF) {
> +		switch (c) {
> +		case 's':
> +			ret_lsize = 1;
> +			break;
> +		case 'l':
> +			ret_largest = 1;
> +			break;
> +		case 'n':
> +			ret_next = 1;
> +			userino = strtoull(optarg, &p, 10);
> +			break;
> +		default:
> +			return command_usage(&inode_cmd);
> +		}
> +	}
> +

We don't check for invalid combinations anywhere. E.g.:

	xfs_io -c "inode -l <num>" <dev>

... should probably display the command usage, right? I take it the same
goes for the '-s' option.

Also, since I was playing with it:

# ./xfs_io -c "inode -l" /mnt/
Largest inode: 98
# ./xfs_io -c "inode 98" /mnt/
Invalid or non-existent inode number

... what's up with that? ;) FWIW, 98 is one of the internal inodes so
perhaps this is just filtered from bulkstat.

> +	if (((optind < argc) || ret_next) &&
> +	     !(ret_lsize || ret_largest)) {
> +

If we check for invalid combinations as mentioned above, this entire
function can look something like this (pseudocode):

{
	...
	get_opts();
	if (check_cmd_errors())
		command_usage();
	if (find_largest) {
		do_find_largest();
		printf("Largest inode: <ino> (<N>-bit);
		return 0;
	}
	
	do_bulkstat();
	printf("Valid/Next inode: ...");

	return 0;
}

... and so we can eliminate some indentation.

> +		/* Setup bulkreq for -n or [num] only */
> +		bulkreq.lastip = &last;
> +		bulkreq.icount = 1;
> +		bulkreq.ubuffer = &bstat;
> +		bulkreq.ocount = &count;
> +
> +		if (ret_next) {
> +				if ((*p != '\0')) {
> +					printf(_("[num] must be a numeric value\n"));
> +					exitcode = 1;
> +					return 0;
> +				}

Indentation of the above is off.

> +			last = userino;
> +			if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT,
> +					&bulkreq)) {
> +				if (errno == EINVAL)
> +					printf(_("Invalid or non-existent inode\n"));
> +				else
> +					perror("XFS_IOC_FSBULKSTAT");
> +				exitcode = 1;
> +				return 0;
> +			}
> +
> +			if (!count) {
> +			printf(_("There are no further inodes in the filesystem\n"));
> +			return 0;

Indentation.

> +			}
> +
> +			printf(_("Next inode: %llu\n"), bstat.bs_ino);
> +			return 0;
> +		} else {
> +			userino = strtoull(argv[optind], &p, 10);
> +				if ((*p != '\0')) {
> +					printf(_("[num] must be a numeric value\n"));
> +					exitcode = 1;
> +					return 0;
> +				}
> +			last = userino;
> +			if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT_SINGLE,
> +					&bulkreq)) {
> +				if (errno == EINVAL) {
> +					printf(_("Invalid or non-existent inode number\n"));
> +				} else
> +					perror("XFS_IOC_FSBULKSTAT_SINGLE");
> +				exitcode = 1;
> +				return 0;
> +			}
> +			printf(_("Valid inode: %llu\n"), bstat.bs_ino);
> +			return 0;
> +

Both of the above bulkstat cases look quite similar. Could we do
something like the following?

	bulkreq.lastip = ...;
	/* check userino */
	last = userino;
	if (ret_next)
		cmd = XFS_IOC_FSBULKSTAT;
	else
		cmd = XFS_IOC_FSBULKSTAT_SINGLE;
	
	if (xfsctl(...)) {
		...
	}

	printf("Inode: ...", ...);

	return 0;

> +		}
> +	}
> +
> +	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;
> +			memcpy(igrp_rec, igroup,
> +				sizeof(struct xfs_inogrp) * 1024);

Why keep and copy a separate instance of the entire array when we only
care about the last record? It seems to me that igrp_rec could be a
single record.

> +		}
> +
> +		lastgrp--;
> +		lastino = igrp_rec[lastgrp].xi_startino +
> +			  xfs_highbit64(igrp_rec[lastgrp].xi_allocmask);
> +
> +		if (ret_lsize)
> +			printf (_("Largest inode size: %d\n"),
> +				lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> +		else
> +			printf(_("Largest inode: %llu\n"), lastino);
> +

I still don't really get why we have separate -l and -s options here. It
seems to me that the behavior of -l already gives us the information
that -s does. Even if that's not obvious enough, the -l command could
just print out both. For example:

	"Largest inode: 1234 (32-bit)"

Brian

> +		return 0;
> +	}
> +
> +	return command_usage(&inode_cmd);
> +}
> +
>  void
>  open_init(void)
>  {
> @@ -815,6 +965,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 +982,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

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

Re: [PATCH] xfs_io: implement 'inode' command V3

[Carlos Maiolino]

Thanks for the review Brian, I'll walk over it and fix the points you mentioned.
> 
> 
> I still don't really get why we have separate -l and -s options here. It
> seems to me that the behavior of -l already gives us the information
> that -s does. Even if that's not obvious enough, the -l command could
> just print out both. For example:
> 
>       "Largest inode: 1234 (32-bit)"

I agree with you here, but, I'll let Dave answer this question, maybe he had 
some another idea for it that I'm not aware of. 

Cheers.
-- 
Carlos

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

Re: [PATCH] xfs_io: implement 'inode' command V3

[Dave Chinner]

On Fri, Oct 23, 2015 at 11:29:46AM +0200, Carlos Maiolino wrote:
> Thanks for the review Brian, I'll walk over it and fix the points you mentioned.
> > 
> > 
> > I still don't really get why we have separate -l and -s options here. It
> > seems to me that the behavior of -l already gives us the information
> > that -s does. Even if that's not obvious enough, the -l command could
> > just print out both. For example:
> > 
> >       "Largest inode: 1234 (32-bit)"
> 
> I agree with you here, but, I'll let Dave answer this question, maybe he had 
> some another idea for it that I'm not aware of. 

No preference here; all that I was suggesting was that if you want
to know whether inodes are 32/64 bit it doesn't matter what the
largest inode number is.

i.e. "Can I mount this with inode32 and have no problems (yes/no)?"

And it's a lot easier to just query for *any* 64 bit inode than it
is to find the largest inode number...

If you want to combine the two, then that's fine by me.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_io: implement 'inode' command V3

[Carlos Maiolino]

On Wed, Oct 28, 2015 at 11:59:24AM +1100, Dave Chinner wrote:
> On Fri, Oct 23, 2015 at 11:29:46AM +0200, Carlos Maiolino wrote:
> > Thanks for the review Brian, I'll walk over it and fix the points you mentioned.
> > > 
> > > 
> > > I still don't really get why we have separate -l and -s options here. It
> > > seems to me that the behavior of -l already gives us the information
> > > that -s does. Even if that's not obvious enough, the -l command could
> > > just print out both. For example:
> > > 
> > >       "Largest inode: 1234 (32-bit)"
> > 
> > I agree with you here, but, I'll let Dave answer this question, maybe he had 
> > some another idea for it that I'm not aware of. 
> 
> No preference here; all that I was suggesting was that if you want
> to know whether inodes are 32/64 bit it doesn't matter what the
> largest inode number is.
> 
> i.e. "Can I mount this with inode32 and have no problems (yes/no)?"
> 
> And it's a lot easier to just query for *any* 64 bit inode than it
> is to find the largest inode number...
> 
> If you want to combine the two, then that's fine by me.
> 
Honestly, I think having separated commands are easier for that, it doesn't
require users of that the need of parsing the output for example, so, honestly I
believe it's better to have it in different commands, I'm also wondering if
wouldn't be better to return "1" when there are 64bits in the FS and "0" if not,
other than 32/64, so it can be used as a true or false return.

> 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