[PATCH 0/3 V2] xfs_io: implement 'inode' command

[PATCH 0/3 V2] xfs_io: implement 'inode' command

[Carlos Maiolino]

Implements a new xfs_io cmmand, 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 decided to split the implementation in three patches because I thought this
would be easier for review and understand the logic of each argument, mainly
regarding the '-n [num] / [num]' implementation where I'm not sure if I handled
it in a good way.

I also 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.


Carlos Maiolino (3):
  xfs_io: Add inode '-s' command to query physical size of largest inode
  xfs_io: add inode -l argument to return largest inode number
  xfs_io: implement inode '-n' and [num] argument

 io/open.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

-- 
2.4.3

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

[PATCH 1/3] xfs_io: Add inode '-s' command to query physical size of largest inode

[Carlos Maiolino]

Add a new inode command to xfs_io, which aims to implement a tool for query
information about inode usage of the filesystem.

This patch implements '-s' inode option, which query the filesystem for the
physical size of the largest filesystem inode

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

diff --git a/io/open.c b/io/open.c
index ac5a5e0..6a794ba 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,74 @@ statfs_f(
 	return 0;
 }
 
+static void
+inode_help(void)
+{
+	printf(_(
+"\n"
+"Query physical information about the inode"
+"\n"
+" -s	-- Returns the physical size (in bits) of the\n"
+"	   largest inode number in the filesystem\n"
+"\n"));
+}
+
+static int
+inode_f(
+	  int			argc,
+	  char			**argv)
+{
+	__s32			count = 0;
+	__s32			lastgrp = 0;
+	__u64			last = 0;
+	__u64			lastino = 0;
+	struct xfs_inogrp	igroup[1024];
+	struct xfs_fsop_bulkreq	bulkreq;
+	int			c;
+	int			ret_lsize = 0;
+
+	bulkreq.lastip = &last;
+	bulkreq.icount = 1024; /* maybe an user-defined value!? */
+	bulkreq.ubuffer = &igroup;
+	bulkreq.ocount = &count;
+
+	while ((c = getopt(argc, argv, "s")) != EOF) {
+		switch (c) {
+		case 's':
+			ret_lsize = 1;
+			break;
+		default:
+			return command_usage(&inode_cmd);
+		}
+	}
+
+	if (ret_lsize) {
+		for (;;) {
+			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
+					&bulkreq)) {
+				perror("XFS_IOC_FSINUMBERS");
+				exitcode = 1;
+				return 0;
+			}
+			if (count < XFS_INODES_PER_CHUNK && count > 0)
+				lastgrp = count;
+			if (!count)
+				break;
+		}
+
+		lastgrp--;
+		lastino = igroup[lastgrp].xi_startino +
+			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
+
+		printf (_("Largest inode size: %d\n"),
+			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
+
+	}
+
+
+	return 0;
+}
+
 void
 open_init(void)
 {
@@ -815,6 +885,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]");
+	inode_cmd.argmin = 1;
+	inode_cmd.argmax = 1;
+	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 +902,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

[PATCH 2/3] xfs_io: add inode -l argument to return largest inode number

[Carlos Maiolino]

Implements '-l' argument in inode command, returning to the user, the largest
inode allocated and used in the filesystem.

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

diff --git a/io/open.c b/io/open.c
index 6a794ba..57ff0bf 100644
--- a/io/open.c
+++ b/io/open.c
@@ -759,6 +759,7 @@ inode_help(void)
 "\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"));
@@ -777,23 +778,27 @@ inode_f(
 	struct xfs_fsop_bulkreq	bulkreq;
 	int			c;
 	int			ret_lsize = 0;
+	int			ret_largest = 0;
 
 	bulkreq.lastip = &last;
 	bulkreq.icount = 1024; /* maybe an user-defined value!? */
 	bulkreq.ubuffer = &igroup;
 	bulkreq.ocount = &count;
 
-	while ((c = getopt(argc, argv, "s")) != EOF) {
+	while ((c = getopt(argc, argv, "sl")) != EOF) {
 		switch (c) {
 		case 's':
 			ret_lsize = 1;
 			break;
+		case 'l':
+			ret_largest = 1;
+			break;
 		default:
 			return command_usage(&inode_cmd);
 		}
 	}
 
-	if (ret_lsize) {
+	if (ret_lsize || ret_largest) {
 		for (;;) {
 			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
 					&bulkreq)) {
@@ -811,8 +816,11 @@ inode_f(
 		lastino = igroup[lastgrp].xi_startino +
 			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
 
-		printf (_("Largest inode size: %d\n"),
-			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
+		if (ret_lsize)
+			printf (_("Largest inode size: %d\n"),
+				lastino > XFS_MAXINUMBER_32 ? 64 : 32);
+		else
+			printf(_("Largest inode: %llu\n"), lastino);
 
 	}
 
@@ -887,7 +895,7 @@ open_init(void)
 
 	inode_cmd.name = "inode";
 	inode_cmd.cfunc = inode_f;
-	inode_cmd.args = _("[-s]");
+	inode_cmd.args = _("[-s | -l]");
 	inode_cmd.argmin = 1;
 	inode_cmd.argmax = 1;
 	inode_cmd.flags = CMD_NOMAP_OK;
-- 
2.4.3

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

[PATCH 3/3] xfs_io: implement inode '-n' and [num] argument

[Carlos Maiolino]

"-n [num]" argument, will return to the user the next inode valid on the filesystem
after [num].

Using [num] exclusive, will test if the inode [num] is a valid inode in the
filesystem or not.

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

diff --git a/io/open.c b/io/open.c
index 57ff0bf..9f68de0 100644
--- a/io/open.c
+++ b/io/open.c
@@ -762,6 +762,8 @@ inode_help(void)
 " -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"));
 }
 
@@ -774,18 +776,19 @@ inode_f(
 	__s32			lastgrp = 0;
 	__u64			last = 0;
 	__u64			lastino = 0;
-	struct xfs_inogrp	igroup[1024];
-	struct xfs_fsop_bulkreq	bulkreq;
+	__u64			userino = 0;
+	char			*p;
 	int			c;
 	int			ret_lsize = 0;
 	int			ret_largest = 0;
+	int			ret_isvalid = 0;
+	int			ret_next = 0;
+	struct xfs_inogrp	igroup[1024];
+	struct xfs_fsop_bulkreq	bulkreq;
+	struct xfs_bstat	bstat;
 
-	bulkreq.lastip = &last;
-	bulkreq.icount = 1024; /* maybe an user-defined value!? */
-	bulkreq.ubuffer = &igroup;
-	bulkreq.ocount = &count;
 
-	while ((c = getopt(argc, argv, "sl")) != EOF) {
+	while ((c = getopt(argc, argv, "sln:")) != EOF) {
 		switch (c) {
 		case 's':
 			ret_lsize = 1;
@@ -793,12 +796,34 @@ inode_f(
 		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)) {
+		ret_isvalid = 1;
+		userino = strtoull(argv[optind], &p, 10);
+	}
+
+	if (userino)
+		if (*p != '\0') {
+			printf("[num] must be a valid number\n");
+			exitcode = 1;
+			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)) {
@@ -806,7 +831,7 @@ inode_f(
 				exitcode = 1;
 				return 0;
 			}
-			if (count < XFS_INODES_PER_CHUNK && count > 0)
+			if (count < 1024 && count > 0)
 				lastgrp = count;
 			if (!count)
 				break;
@@ -822,8 +847,47 @@ inode_f(
 		else
 			printf(_("Largest inode: %llu\n"), lastino);
 
+		return 0;
+	}
+
+	/* Setup bulkreq for -n or [num] only */
+	last = userino;
+	bulkreq.lastip = &last;
+	bulkreq.icount = 1;
+	bulkreq.ubuffer = &bstat;
+	bulkreq.ocount = &count;
+
+	if (ret_next) {
+		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 (!bstat.bs_ino) {
+			printf("There are no further inodes in the filesystem\n");
+			return 0;
+		}
+
+		printf("Next inode: %llu\n", bstat.bs_ino);
+		return 0;
 	}
 
+	if (ret_isvalid) {
+		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;
+	}
 
 	return 0;
 }
@@ -895,9 +959,9 @@ open_init(void)
 
 	inode_cmd.name = "inode";
 	inode_cmd.cfunc = inode_f;
-	inode_cmd.args = _("[-s | -l]");
+	inode_cmd.args = _("[-s | -l | -n] [num]");
 	inode_cmd.argmin = 1;
-	inode_cmd.argmax = 1;
+	inode_cmd.argmax = 2;
 	inode_cmd.flags = CMD_NOMAP_OK;
 	inode_cmd.oneline =
 		_("Query inode number usage in the filesystem");
-- 
2.4.3

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

Re: [PATCH 1/3] xfs_io: Add inode '-s' command to query physical size of largest inode

[Carlos Maiolino]

On Fri, Sep 25, 2015 at 03:07:45PM +0200, Carlos Maiolino wrote:
> Add a new inode command to xfs_io, which aims to implement a tool for query
> information about inode usage of the filesystem.
> 
> This patch implements '-s' inode option, which query the filesystem for the
> physical size of the largest filesystem inode
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  io/open.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index ac5a5e0..6a794ba 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,74 @@ statfs_f(
>  	return 0;
>  }
>  
> +static void
> +inode_help(void)
> +{
> +	printf(_(
> +"\n"
> +"Query physical information about the inode"
> +"\n"
> +" -s	-- Returns the physical size (in bits) of the\n"
> +"	   largest inode number in the filesystem\n"
> +"\n"));
> +}
> +
> +static int
> +inode_f(
> +	  int			argc,
> +	  char			**argv)
> +{
> +	__s32			count = 0;
> +	__s32			lastgrp = 0;
> +	__u64			last = 0;
> +	__u64			lastino = 0;
> +	struct xfs_inogrp	igroup[1024];
> +	struct xfs_fsop_bulkreq	bulkreq;
> +	int			c;
> +	int			ret_lsize = 0;
> +
> +	bulkreq.lastip = &last;
> +	bulkreq.icount = 1024; /* maybe an user-defined value!? */
> +	bulkreq.ubuffer = &igroup;
> +	bulkreq.ocount = &count;
> +
> +	while ((c = getopt(argc, argv, "s")) != EOF) {
> +		switch (c) {
> +		case 's':
> +			ret_lsize = 1;
> +			break;
> +		default:
> +			return command_usage(&inode_cmd);
> +		}
> +	}
> +
> +	if (ret_lsize) {
> +		for (;;) {
> +			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
> +					&bulkreq)) {
> +				perror("XFS_IOC_FSINUMBERS");
> +				exitcode = 1;
> +				return 0;
> +			}
> +			if (count < XFS_INODES_PER_CHUNK && count > 0)
					^^^ bummer :( I forgot to remove this
					macro before commiting this patch, it
					has been changed to 1024 in the patch 3/3
					will fix this up before sending V3,
					after getting more reviews on these patches

> +				lastgrp = count;
> +			if (!count)
> +				break;
> +		}
> +
> +		lastgrp--;
> +		lastino = igroup[lastgrp].xi_startino +
> +			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
> +
> +		printf (_("Largest inode size: %d\n"),
> +			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> +
> +	}
> +
> +
> +	return 0;
> +}
> +
>  void
>  open_init(void)
>  {
> @@ -815,6 +885,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]");
> +	inode_cmd.argmin = 1;
> +	inode_cmd.argmax = 1;
> +	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 +902,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

Re: [PATCH 1/3] xfs_io: Add inode '-s' command to query physical size of largest inode

[Brian Foster]

On Fri, Sep 25, 2015 at 03:07:45PM +0200, Carlos Maiolino wrote:
> Add a new inode command to xfs_io, which aims to implement a tool for query
> information about inode usage of the filesystem.
> 
> This patch implements '-s' inode option, which query the filesystem for the
> physical size of the largest filesystem inode
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  io/open.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index ac5a5e0..6a794ba 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,74 @@ statfs_f(
>  	return 0;
>  }
>  
> +static void
> +inode_help(void)
> +{
> +	printf(_(
> +"\n"
> +"Query physical information about the inode"
> +"\n"
> +" -s	-- Returns the physical size (in bits) of the\n"
> +"	   largest inode number in the filesystem\n"
> +"\n"));
> +}
> +
> +static int
> +inode_f(
> +	  int			argc,
> +	  char			**argv)
> +{
> +	__s32			count = 0;
> +	__s32			lastgrp = 0;
> +	__u64			last = 0;
> +	__u64			lastino = 0;
> +	struct xfs_inogrp	igroup[1024];
> +	struct xfs_fsop_bulkreq	bulkreq;
> +	int			c;
> +	int			ret_lsize = 0;
> +
> +	bulkreq.lastip = &last;
> +	bulkreq.icount = 1024; /* maybe an user-defined value!? */
> +	bulkreq.ubuffer = &igroup;
> +	bulkreq.ocount = &count;
> +
> +	while ((c = getopt(argc, argv, "s")) != EOF) {
> +		switch (c) {
> +		case 's':
> +			ret_lsize = 1;
> +			break;
> +		default:
> +			return command_usage(&inode_cmd);
> +		}
> +	}
> +
> +	if (ret_lsize) {

It seems a little strange to me to do nothing by default, even if that
means printing a warning to ask the user to specify one of N required
options.

> +		for (;;) {
> +			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
> +					&bulkreq)) {
> +				perror("XFS_IOC_FSINUMBERS");
> +				exitcode = 1;
> +				return 0;
> +			}
> +			if (count < XFS_INODES_PER_CHUNK && count > 0)
> +				lastgrp = count;

Is this assuming that the number of inodes in the fs is not a multiple
of XFS_INODES_PER_CHUNK? It looks like this is never set, for example,
if the fs has exactly XFS_INODES_PER_CHUNK inodes allocated.

I suspect what we want to do here is break the loop as soon as something
less than the full buffer is returned, then use the count and record
size to determine which record has the largest ino and go from there..?
I also don't know what happens if the user buffer is passed to a
subsequent command that returns count == 0. Is it still valid?

A more simple algorithm might be to copy the last record in igroup to
another local xfs_inogrp variable in each iteration and just have the
subsequent code refer to that record (or even update lastino here and
then just print it below). Just a thought.

Brian

> +			if (!count)
> +				break;
> +		}
> +
> +		lastgrp--;
> +		lastino = igroup[lastgrp].xi_startino +
> +			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
> +
> +		printf (_("Largest inode size: %d\n"),
> +			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> +
> +	}
> +
> +
> +	return 0;
> +}
> +
>  void
>  open_init(void)
>  {
> @@ -815,6 +885,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]");
> +	inode_cmd.argmin = 1;
> +	inode_cmd.argmax = 1;
> +	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 +902,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 2/3] xfs_io: add inode -l argument to return largest inode number

[Brian Foster]

On Fri, Sep 25, 2015 at 03:07:46PM +0200, Carlos Maiolino wrote:
> Implements '-l' argument in inode command, returning to the user, the largest
> inode allocated and used in the filesystem.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  io/open.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/io/open.c b/io/open.c
> index 6a794ba..57ff0bf 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -759,6 +759,7 @@ inode_help(void)
>  "\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"));
> @@ -777,23 +778,27 @@ inode_f(
>  	struct xfs_fsop_bulkreq	bulkreq;
>  	int			c;
>  	int			ret_lsize = 0;
> +	int			ret_largest = 0;
>  
>  	bulkreq.lastip = &last;
>  	bulkreq.icount = 1024; /* maybe an user-defined value!? */
>  	bulkreq.ubuffer = &igroup;
>  	bulkreq.ocount = &count;
>  
> -	while ((c = getopt(argc, argv, "s")) != EOF) {
> +	while ((c = getopt(argc, argv, "sl")) != EOF) {
>  		switch (c) {
>  		case 's':
>  			ret_lsize = 1;
>  			break;
> +		case 'l':
> +			ret_largest = 1;
> +			break;
>  		default:
>  			return command_usage(&inode_cmd);
>  		}
>  	}
>  
> -	if (ret_lsize) {
> +	if (ret_lsize || ret_largest) {
>  		for (;;) {
>  			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
>  					&bulkreq)) {
> @@ -811,8 +816,11 @@ inode_f(
>  		lastino = igroup[lastgrp].xi_startino +
>  			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
>  
> -		printf (_("Largest inode size: %d\n"),
> -			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> +		if (ret_lsize)
> +			printf (_("Largest inode size: %d\n"),
> +				lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> +		else
> +			printf(_("Largest inode: %llu\n"), lastino);

Hmm, do we need the -s option if we have -l to print the actual largest
inode number?

Brian

>  
>  	}
>  
> @@ -887,7 +895,7 @@ open_init(void)
>  
>  	inode_cmd.name = "inode";
>  	inode_cmd.cfunc = inode_f;
> -	inode_cmd.args = _("[-s]");
> +	inode_cmd.args = _("[-s | -l]");
>  	inode_cmd.argmin = 1;
>  	inode_cmd.argmax = 1;
>  	inode_cmd.flags = CMD_NOMAP_OK;
> -- 
> 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 3/3] xfs_io: implement inode '-n' and [num] argument

[Brian Foster]

On Fri, Sep 25, 2015 at 03:07:47PM +0200, Carlos Maiolino wrote:
> "-n [num]" argument, will return to the user the next inode valid on the filesystem
> after [num].
> 
> Using [num] exclusive, will test if the inode [num] is a valid inode in the
> filesystem or not.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  io/open.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/io/open.c b/io/open.c
> index 57ff0bf..9f68de0 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -762,6 +762,8 @@ inode_help(void)
>  " -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"));
>  }
>  
> @@ -774,18 +776,19 @@ inode_f(
>  	__s32			lastgrp = 0;
>  	__u64			last = 0;
>  	__u64			lastino = 0;
> -	struct xfs_inogrp	igroup[1024];
> -	struct xfs_fsop_bulkreq	bulkreq;
> +	__u64			userino = 0;
> +	char			*p;
>  	int			c;
>  	int			ret_lsize = 0;
>  	int			ret_largest = 0;
> +	int			ret_isvalid = 0;
> +	int			ret_next = 0;
> +	struct xfs_inogrp	igroup[1024];
> +	struct xfs_fsop_bulkreq	bulkreq;
> +	struct xfs_bstat	bstat;
>  
> -	bulkreq.lastip = &last;
> -	bulkreq.icount = 1024; /* maybe an user-defined value!? */
> -	bulkreq.ubuffer = &igroup;
> -	bulkreq.ocount = &count;
>  
> -	while ((c = getopt(argc, argv, "sl")) != EOF) {
> +	while ((c = getopt(argc, argv, "sln:")) != EOF) {
>  		switch (c) {
>  		case 's':
>  			ret_lsize = 1;
> @@ -793,12 +796,34 @@ inode_f(
>  		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)) {
> +		ret_isvalid = 1;
> +		userino = strtoull(argv[optind], &p, 10);
> +	}

So this appears to be the default behavior (validate whether an inode
exists). Perhaps this functionality should come first since that's the
core behavior for the command.

> +
> +	if (userino)
> +		if (*p != '\0') {
> +			printf("[num] must be a valid number\n");
> +			exitcode = 1;
> +			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)) {
> @@ -806,7 +831,7 @@ inode_f(
>  				exitcode = 1;
>  				return 0;
>  			}
> -			if (count < XFS_INODES_PER_CHUNK && count > 0)
> +			if (count < 1024 && count > 0)
>  				lastgrp = count;

Ok, that sort of addresses my question on patch 1. I guess this is a
record count rather than an inode count as well. In that case, what
happens if the fs has an exact multiple of 1024 inode records?

BTW, I think this should probably be set correctly when it is introduced
rather than set to a value and changed in a subsequent patch.

>  			if (!count)
>  				break;
> @@ -822,8 +847,47 @@ inode_f(
>  		else
>  			printf(_("Largest inode: %llu\n"), lastino);
>  
> +		return 0;
> +	}
> +
> +	/* Setup bulkreq for -n or [num] only */
> +	last = userino;
> +	bulkreq.lastip = &last;
> +	bulkreq.icount = 1;
> +	bulkreq.ubuffer = &bstat;
> +	bulkreq.ocount = &count;
> +
> +	if (ret_next) {
> +		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 (!bstat.bs_ino) {
> +			printf("There are no further inodes in the filesystem\n");
> +			return 0;
> +		}

The above should technically check the output count rather than the
inode number, right?

> +
> +		printf("Next inode: %llu\n", bstat.bs_ino);
> +		return 0;
>  	}
>  
> +	if (ret_isvalid) {
> +		if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq)) {
> +			if (errno == EINVAL) {
> +				printf("Invalid or non-existent inode number\n");

Is EINVAL returned in the non-existent case or ENOENT?

Brian

> +			} else
> +				perror("XFS_IOC_FSBULKSTAT_SINGLE");
> +			exitcode = 1;
> +			return 0;
> +		}
> +		printf("Valid inode: %llu\n", bstat.bs_ino);
> +		return 0;
> +	}
>  
>  	return 0;
>  }
> @@ -895,9 +959,9 @@ open_init(void)
>  
>  	inode_cmd.name = "inode";
>  	inode_cmd.cfunc = inode_f;
> -	inode_cmd.args = _("[-s | -l]");
> +	inode_cmd.args = _("[-s | -l | -n] [num]");
>  	inode_cmd.argmin = 1;
> -	inode_cmd.argmax = 1;
> +	inode_cmd.argmax = 2;
>  	inode_cmd.flags = CMD_NOMAP_OK;
>  	inode_cmd.oneline =
>  		_("Query inode number usage in the filesystem");
> -- 
> 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 2/3] xfs_io: add inode -l argument to return largest inode number

[Carlos Maiolino]

On Tue, Oct 06, 2015 at 01:00:39PM -0400, Brian Foster wrote:
> On Fri, Sep 25, 2015 at 03:07:46PM +0200, Carlos Maiolino wrote:
> > Implements '-l' argument in inode command, returning to the user, the largest
> > inode allocated and used in the filesystem.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  io/open.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index 6a794ba..57ff0bf 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> > @@ -759,6 +759,7 @@ inode_help(void)
> >  "\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"));
> > @@ -777,23 +778,27 @@ inode_f(
> >  	struct xfs_fsop_bulkreq	bulkreq;
> >  	int			c;
> >  	int			ret_lsize = 0;
> > +	int			ret_largest = 0;
> >  
> >  	bulkreq.lastip = &last;
> >  	bulkreq.icount = 1024; /* maybe an user-defined value!? */
> >  	bulkreq.ubuffer = &igroup;
> >  	bulkreq.ocount = &count;
> >  
> > -	while ((c = getopt(argc, argv, "s")) != EOF) {
> > +	while ((c = getopt(argc, argv, "sl")) != EOF) {
> >  		switch (c) {
> >  		case 's':
> >  			ret_lsize = 1;
> >  			break;
> > +		case 'l':
> > +			ret_largest = 1;
> > +			break;
> >  		default:
> >  			return command_usage(&inode_cmd);
> >  		}
> >  	}
> >  
> > -	if (ret_lsize) {
> > +	if (ret_lsize || ret_largest) {
> >  		for (;;) {
> >  			if (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS,
> >  					&bulkreq)) {
> > @@ -811,8 +816,11 @@ inode_f(
> >  		lastino = igroup[lastgrp].xi_startino +
> >  			  xfs_highbit64(igroup[lastgrp].xi_allocmask);
> >  
> > -		printf (_("Largest inode size: %d\n"),
> > -			 lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> > +		if (ret_lsize)
> > +			printf (_("Largest inode size: %d\n"),
> > +				lastino > XFS_MAXINUMBER_32 ? 64 : 32);
> > +		else
> > +			printf(_("Largest inode: %llu\n"), lastino);
> 
> Hmm, do we need the -s option if we have -l to print the actual largest
> inode number?
> 

Well, I understand your question, my point here was to implement the options
Dave suggested, but I'm not sure if he had in mind anything else that checking
the return value size of -l would not work as -s argument.

> Brian
> 
> >  
> >  	}
> >  
> > @@ -887,7 +895,7 @@ open_init(void)
> >  
> >  	inode_cmd.name = "inode";
> >  	inode_cmd.cfunc = inode_f;
> > -	inode_cmd.args = _("[-s]");
> > +	inode_cmd.args = _("[-s | -l]");
> >  	inode_cmd.argmin = 1;
> >  	inode_cmd.argmax = 1;
> >  	inode_cmd.flags = CMD_NOMAP_OK;
> > -- 
> > 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

Re: [PATCH 3/3] xfs_io: implement inode '-n' and [num] argument

[Carlos Maiolino]

On Tue, Oct 06, 2015 at 01:00:56PM -0400, Brian Foster wrote:
> On Fri, Sep 25, 2015 at 03:07:47PM +0200, Carlos Maiolino wrote:
> > "-n [num]" argument, will return to the user the next inode valid on the filesystem
> > after [num].
> > 
> > Using [num] exclusive, will test if the inode [num] is a valid inode in the
> > filesystem or not.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  io/open.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 74 insertions(+), 10 deletions(-)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index 57ff0bf..9f68de0 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> > @@ -762,6 +762,8 @@ inode_help(void)
> >  " -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"));
> >  }
> >  
> > @@ -774,18 +776,19 @@ inode_f(
> >  	__s32			lastgrp = 0;
> >  	__u64			last = 0;
> >  	__u64			lastino = 0;
> > -	struct xfs_inogrp	igroup[1024];
> > -	struct xfs_fsop_bulkreq	bulkreq;
> > +	__u64			userino = 0;
> > +	char			*p;
> >  	int			c;
> >  	int			ret_lsize = 0;
> >  	int			ret_largest = 0;
> > +	int			ret_isvalid = 0;
> > +	int			ret_next = 0;
> > +	struct xfs_inogrp	igroup[1024];
> > +	struct xfs_fsop_bulkreq	bulkreq;
> > +	struct xfs_bstat	bstat;
> >  
> > -	bulkreq.lastip = &last;
> > -	bulkreq.icount = 1024; /* maybe an user-defined value!? */
> > -	bulkreq.ubuffer = &igroup;
> > -	bulkreq.ocount = &count;
> >  
> > -	while ((c = getopt(argc, argv, "sl")) != EOF) {
> > +	while ((c = getopt(argc, argv, "sln:")) != EOF) {
> >  		switch (c) {
> >  		case 's':
> >  			ret_lsize = 1;
> > @@ -793,12 +796,34 @@ inode_f(
> >  		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)) {
> > +		ret_isvalid = 1;
> > +		userino = strtoull(argv[optind], &p, 10);
> > +	}
> 
> So this appears to be the default behavior (validate whether an inode
> exists). Perhaps this functionality should come first since that's the
> core behavior for the command.
> 

Hmm, I don't think so, I need getopt() to setup optind for this.

> > +
> > +	if (userino)
> > +		if (*p != '\0') {
> > +			printf("[num] must be a valid number\n");
> > +			exitcode = 1;
> > +			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)) {
> > @@ -806,7 +831,7 @@ inode_f(
> >  				exitcode = 1;
> >  				return 0;
> >  			}
> > -			if (count < XFS_INODES_PER_CHUNK && count > 0)
> > +			if (count < 1024 && count > 0)
> >  				lastgrp = count;
> 
> Ok, that sort of addresses my question on patch 1. I guess this is a
> record count rather than an inode count as well. In that case, what
> happens if the fs has an exact multiple of 1024 inode records?
> 
Yes, it's a record count, each record contains a single inode chunk.
regarding the exactly multiple of 1024 chunks, AFAIK it will fit all the 1024
records in a single array, which is exactly the size of the array I'm using
here, and, next call to xfsctl, will return a 0 records count, making the look
to exit.

> BTW, I think this should probably be set correctly when it is introduced
> rather than set to a value and changed in a subsequent patch.

Yes, I just forgot to change this in the first patch, see my comment in patch 1.

> 
> >  			if (!count)
> >  				break;
> > @@ -822,8 +847,47 @@ inode_f(
> >  		else
> >  			printf(_("Largest inode: %llu\n"), lastino);
> >  
> > +		return 0;
> > +	}
> > +
> > +	/* Setup bulkreq for -n or [num] only */
> > +	last = userino;
> > +	bulkreq.lastip = &last;
> > +	bulkreq.icount = 1;
> > +	bulkreq.ubuffer = &bstat;
> > +	bulkreq.ocount = &count;
> > +
> > +	if (ret_next) {
> > +		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 (!bstat.bs_ino) {
> > +			printf("There are no further inodes in the filesystem\n");
> > +			return 0;
> > +		}
> 
> The above should technically check the output count rather than the
> inode number, right?
> 
If I use the inode count, I can get an 'allocated but free' inode, which is not
the intention here.
 
> > +
> > +		printf("Next inode: %llu\n", bstat.bs_ino);
> > +		return 0;
> >  	}
> >  
> > +	if (ret_isvalid) {
> > +		if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq)) {
> > +			if (errno == EINVAL) {
> > +				printf("Invalid or non-existent inode number\n");
> 
> Is EINVAL returned in the non-existent case or ENOENT?
>

I'll check this inside kernel, but I'm not sure if there is any distinction. If
the inode passed to xfsctl is incalid, EINVAL will be returned.

 
> Brian
> 
> > +			} else
> > +				perror("XFS_IOC_FSBULKSTAT_SINGLE");
> > +			exitcode = 1;
> > +			return 0;
> > +		}
> > +		printf("Valid inode: %llu\n", bstat.bs_ino);
> > +		return 0;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -895,9 +959,9 @@ open_init(void)
> >  
> >  	inode_cmd.name = "inode";
> >  	inode_cmd.cfunc = inode_f;
> > -	inode_cmd.args = _("[-s | -l]");
> > +	inode_cmd.args = _("[-s | -l | -n] [num]");
> >  	inode_cmd.argmin = 1;
> > -	inode_cmd.argmax = 1;
> > +	inode_cmd.argmax = 2;
> >  	inode_cmd.flags = CMD_NOMAP_OK;
> >  	inode_cmd.oneline =
> >  		_("Query inode number usage in the filesystem");
> > -- 
> > 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

Re: [PATCH 3/3] xfs_io: implement inode '-n' and [num] argument

[Brian Foster]

On Fri, Oct 09, 2015 at 10:33:13AM +0200, Carlos Maiolino wrote:
> On Tue, Oct 06, 2015 at 01:00:56PM -0400, Brian Foster wrote:
> > On Fri, Sep 25, 2015 at 03:07:47PM +0200, Carlos Maiolino wrote:
> > > "-n [num]" argument, will return to the user the next inode valid on the filesystem
> > > after [num].
> > > 
> > > Using [num] exclusive, will test if the inode [num] is a valid inode in the
> > > filesystem or not.
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > >  io/open.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 74 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/io/open.c b/io/open.c
> > > index 57ff0bf..9f68de0 100644
> > > --- a/io/open.c
> > > +++ b/io/open.c
...
> > >  
> > > +	if ((optind < argc) && !(ret_next || ret_lsize || ret_largest)) {
> > > +		ret_isvalid = 1;
> > > +		userino = strtoull(argv[optind], &p, 10);
> > > +	}
> > 
> > So this appears to be the default behavior (validate whether an inode
> > exists). Perhaps this functionality should come first since that's the
> > core behavior for the command.
> > 
> 
> Hmm, I don't think so, I need getopt() to setup optind for this.
> 

I don't see how that matters. That code can stay in the first patch. I'm
just saying patch 1 should probably implement the core/default
functionality, and obviously whatever supporting code is necessary to
make that happen. For example, that could mean that the above
ret_isvalid = 1 block goes away, the code executes in this mode by
default, and the subsequent patches implement alternate branches as
necessary to alter behavior.

In other words, the (pseudo)code can start off looking like this:

	userino = ...;

	...

	bulkreq = ...
	xfsctl(..., XFS_IOC_FSBULKSTAT_SINGLE, ...);
	printf("Valid inode: ...");
	return 0;

... then patch 2 comes along an adds a next option:

	int cmd = XFS_IOC_FSBULKSTAT_SINGLE;

	while (getopt() = ...) {
		if (next)
			cmd = XFS_IOC_FSBULKSTAT;
	}
	userino = ...;

	...

	bulkreq = ...
	xfsctl(..., cmd, ...);
	printf("%s inode: ...", next ? "Next" : "Valid", ...);
	return 0;

... and so on. Patch 3 comes along and adds more command line options
and an alternate FSNUMBERS branch before the bulkstat xfsctl(). That
branch ends with a return 0, so there's no need to put the core
mechanism bits above into an 'if (ret_isvalid).'

The alternative is to just squash everything to one patch, which is
probably reasonable too. I still think the end result can be simplified
and reduced to something like the above though.

> > > +
> > > +	if (userino)
> > > +		if (*p != '\0') {
> > > +			printf("[num] must be a valid number\n");
> > > +			exitcode = 1;
> > > +			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)) {
> > > @@ -806,7 +831,7 @@ inode_f(
> > >  				exitcode = 1;
> > >  				return 0;
> > >  			}
> > > -			if (count < XFS_INODES_PER_CHUNK && count > 0)
> > > +			if (count < 1024 && count > 0)
> > >  				lastgrp = count;
> > 
> > Ok, that sort of addresses my question on patch 1. I guess this is a
> > record count rather than an inode count as well. In that case, what
> > happens if the fs has an exact multiple of 1024 inode records?
> > 
> Yes, it's a record count, each record contains a single inode chunk.
> regarding the exactly multiple of 1024 chunks, AFAIK it will fit all the 1024
> records in a single array, which is exactly the size of the array I'm using
> here, and, next call to xfsctl, will return a 0 records count, making the look
> to exit.
> 

Ok, that's what I would expect up to that point. To be more clear, when
is lastgrp ever set? Further, what happens if xfsctl() somewhere down
the road decides to memset(..., 0, ...) bulkreq.ubuffer (for example)
when count is set to 0?

For example, here's a quick experiment on an fs with precisely 1024
inode records:

# ./io/xfs_io -c "inode -l" /mnt/
Largest inode: 1070
# find /mnt/ -inum 1070 -print
#

Oops! :) After adding a few more records:

# ./io/xfs_io -c "inode -l" /mnt/
Largest inode: 971014
# find /mnt/ -inum 971014 -print
/mnt/tmp/128
#

> > BTW, I think this should probably be set correctly when it is introduced
> > rather than set to a value and changed in a subsequent patch.
> 
> Yes, I just forgot to change this in the first patch, see my comment in patch 1.
> 
> > 
> > >  			if (!count)
> > >  				break;
> > > @@ -822,8 +847,47 @@ inode_f(
> > >  		else
> > >  			printf(_("Largest inode: %llu\n"), lastino);
> > >  
> > > +		return 0;
> > > +	}
> > > +
> > > +	/* Setup bulkreq for -n or [num] only */
> > > +	last = userino;
> > > +	bulkreq.lastip = &last;
> > > +	bulkreq.icount = 1;
> > > +	bulkreq.ubuffer = &bstat;
> > > +	bulkreq.ocount = &count;
> > > +
> > > +	if (ret_next) {
> > > +		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 (!bstat.bs_ino) {
> > > +			printf("There are no further inodes in the filesystem\n");
> > > +			return 0;
> > > +		}
> > 
> > The above should technically check the output count rather than the
> > inode number, right?
> > 
> If I use the inode count, I can get an 'allocated but free' inode, which is not
> the intention here.
>  

I don't think bulkstat returns unused (but allocated) inodes. Most of
the inode information would be invalid/undefined. Indeed, from
xfs_bulkstat_ag_ichunk():

                /* Skip if this inode is free */
                if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free)
                        continue;

Brian

> > > +
> > > +		printf("Next inode: %llu\n", bstat.bs_ino);
> > > +		return 0;
> > >  	}
> > >  
> > > +	if (ret_isvalid) {
> > > +		if (xfsctl(file->name, file->fd, XFS_IOC_FSBULKSTAT_SINGLE, &bulkreq)) {
> > > +			if (errno == EINVAL) {
> > > +				printf("Invalid or non-existent inode number\n");
> > 
> > Is EINVAL returned in the non-existent case or ENOENT?
> >
> 
> I'll check this inside kernel, but I'm not sure if there is any distinction. If
> the inode passed to xfsctl is incalid, EINVAL will be returned.
> 
>  
> > Brian
> > 
> > > +			} else
> > > +				perror("XFS_IOC_FSBULKSTAT_SINGLE");
> > > +			exitcode = 1;
> > > +			return 0;
> > > +		}
> > > +		printf("Valid inode: %llu\n", bstat.bs_ino);
> > > +		return 0;
> > > +	}
> > >  
> > >  	return 0;
> > >  }
> > > @@ -895,9 +959,9 @@ open_init(void)
> > >  
> > >  	inode_cmd.name = "inode";
> > >  	inode_cmd.cfunc = inode_f;
> > > -	inode_cmd.args = _("[-s | -l]");
> > > +	inode_cmd.args = _("[-s | -l | -n] [num]");
> > >  	inode_cmd.argmin = 1;
> > > -	inode_cmd.argmax = 1;
> > > +	inode_cmd.argmax = 2;
> > >  	inode_cmd.flags = CMD_NOMAP_OK;
> > >  	inode_cmd.oneline =
> > >  		_("Query inode number usage in the filesystem");
> > > -- 
> > > 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

Re: [PATCH 3/3] xfs_io: implement inode '-n' and [num] argument

[Carlos Maiolino]

Hey

> > 
> > Hmm, I don't think so, I need getopt() to setup optind for this.
> > 
> 
> I don't see how that matters. That code can stay in the first patch. I'm
> just saying patch 1 should probably implement the core/default
> functionality, and obviously whatever supporting code is necessary to
> make that happen. For example, that could mean that the above
> ret_isvalid = 1 block goes away, the code executes in this mode by
> default, and the subsequent patches implement alternate branches as
> necessary to alter behavior.
> 

Right, I think I misunderstood your previous comment, when you said about this
going first, I thought about code ordering, not patch ordering. In this point I
agree with you.

> In other words, the (pseudo)code can start off looking like this:
> 
> 	userino = ...;
> 
> 	...
> 
> 	bulkreq = ...
> 	xfsctl(..., XFS_IOC_FSBULKSTAT_SINGLE, ...);
> 	printf("Valid inode: ...");
> 	return 0;
> 
> ... then patch 2 comes along an adds a next option:
> 
> 	int cmd = XFS_IOC_FSBULKSTAT_SINGLE;
> 
> 	while (getopt() = ...) {
> 		if (next)
> 			cmd = XFS_IOC_FSBULKSTAT;
> 	}
> 	userino = ...;
> 
> 	...
> 
> 	bulkreq = ...
> 	xfsctl(..., cmd, ...);
> 	printf("%s inode: ...", next ? "Next" : "Valid", ...);
> 	return 0;
> 
> ... and so on. Patch 3 comes along and adds more command line options
> and an alternate FSNUMBERS branch before the bulkstat xfsctl(). That
> branch ends with a return 0, so there's no need to put the core
> mechanism bits above into an 'if (ret_isvalid).'
> 
Makes sense

> The alternative is to just squash everything to one patch, which is
> probably reasonable too. I still think the end result can be simplified
> and reduced to something like the above though.
> 
> > > > +
> > > > +	if (userino)
> > > > +		if (*p != '\0') {
> > > > +			printf("[num] must be a valid number\n");
> > > > +			exitcode = 1;
> > > > +			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)) {
> > > > @@ -806,7 +831,7 @@ inode_f(
> > > >  				exitcode = 1;
> > > >  				return 0;
> > > >  			}
> > > > -			if (count < XFS_INODES_PER_CHUNK && count > 0)
> > > > +			if (count < 1024 && count > 0)
> > > >  				lastgrp = count;
> > > 
> > > Ok, that sort of addresses my question on patch 1. I guess this is a
> > > record count rather than an inode count as well. In that case, what
> > > happens if the fs has an exact multiple of 1024 inode records?
> > > 
> > Yes, it's a record count, each record contains a single inode chunk.
> > regarding the exactly multiple of 1024 chunks, AFAIK it will fit all the 1024
> > records in a single array, which is exactly the size of the array I'm using
> > here, and, next call to xfsctl, will return a 0 records count, making the look
> > to exit.
> > 
> 
> Ok, that's what I would expect up to that point. To be more clear, when
> is lastgrp ever set? Further, what happens if xfsctl() somewhere down
> the road decides to memset(..., 0, ...) bulkreq.ubuffer (for example)
> when count is set to 0?
> 
> For example, here's a quick experiment on an fs with precisely 1024
> inode records:
> 
> # ./io/xfs_io -c "inode -l" /mnt/
> Largest inode: 1070
> # find /mnt/ -inum 1070 -print
> #
> 
> Oops! :) After adding a few more records:
> 
> # ./io/xfs_io -c "inode -l" /mnt/
> Largest inode: 971014
> # find /mnt/ -inum 971014 -print
> /mnt/tmp/128
> #
> 
> > > BTW, I think this should probably be set correctly when it is introduced
> > > rather than set to a value and changed in a subsequent patch.
> > 
> > Yes, I just forgot to change this in the first patch, see my comment in patch 1.
> > 
> > > 
> > > >  			if (!count)
> > > >  				break;
> > > > @@ -822,8 +847,47 @@ inode_f(
> > > >  		else
> > > >  			printf(_("Largest inode: %llu\n"), lastino);
> > > >  
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	/* Setup bulkreq for -n or [num] only */
> > > > +	last = userino;
> > > > +	bulkreq.lastip = &last;
> > > > +	bulkreq.icount = 1;
> > > > +	bulkreq.ubuffer = &bstat;
> > > > +	bulkreq.ocount = &count;
> > > > +
> > > > +	if (ret_next) {
> > > > +		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 (!bstat.bs_ino) {
> > > > +			printf("There are no further inodes in the filesystem\n");
> > > > +			return 0;
> > > > +		}
> > > 
> > > The above should technically check the output count rather than the
> > > inode number, right?
> > > 
> > If I use the inode count, I can get an 'allocated but free' inode, which is not
> > the intention here.
> >  
> 
> I don't think bulkstat returns unused (but allocated) inodes. Most of
> the inode information would be invalid/undefined. Indeed, from
> xfs_bulkstat_ag_ichunk():
> 
>                 /* Skip if this inode is free */
>                 if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free)
>                         continue;
> 
> Brian
> 

I'll check the another points on Monday, thanks for the review Brian.

have a good weekend

-- 
Carlos

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