[PATCH] xfs_io: Implement inodes64 command

[PATCH] xfs_io: Implement inodes64 command

[Carlos Maiolino]

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;
+
+	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)");
+	exitcode = 1;
+	return 0;
+}
+
 void
 open_init(void)
 {
@@ -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");
+
 	add_command(&open_cmd);
 	add_command(&stat_cmd);
 	add_command(&close_cmd);
@@ -822,4 +860,5 @@ open_init(void)
 	add_command(&chproj_cmd);
 	add_command(&lsproj_cmd);
 	add_command(&extsize_cmd);
+	add_command(&inodes64_cmd);
 }
-- 
2.4.3

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

Re: [PATCH] xfs_io: Implement inodes64 command

[Eric Sandeen]

On Sep 21, 2015, at 3:56 AM, Carlos Maiolino <cmaiolino@redhat.com> 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;
> +
> +    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)");
> +    exitcode = 1;
> +    return 0;
> +}
> +
> void
> open_init(void)
> {
> @@ -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");

"Check if filesystem contains 64bit inodes"

Still needs a manpage update ;)

Also is it ok to not have inodes64_cmd.help?  (Sorry, on phone right now, can't look, perhaps it's fine)

Thanks,
Eric


> +
>    add_command(&open_cmd);
>    add_command(&stat_cmd);
>    add_command(&close_cmd);
> @@ -822,4 +860,5 @@ open_init(void)
>    add_command(&chproj_cmd);
>    add_command(&lsproj_cmd);
>    add_command(&extsize_cmd);
> +    add_command(&inodes64_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 inodes64 command

[Dave Chinner]

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.

> +
> +	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().

> @@ -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...

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 inodes64 command

[Carlos Maiolino]

On Mon, Sep 21, 2015 at 03:50:21PM -0500, Eric Sandeen wrote:
> On Sep 21, 2015, at 3:56 AM, Carlos Maiolino <cmaiolino@redhat.com> 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;
> > +
> > +    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)");
> > +    exitcode = 1;
> > +    return 0;
> > +}
> > +
> > void
> > open_init(void)
> > {
> > @@ -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");
> 
> "Check if filesystem contains 64bit inodes"
> 
Oh lord.. I completely forgot to fix this, thanks :-)

> Still needs a manpage update ;)
> 
Writing it :)

> Also is it ok to not have inodes64_cmd.help?  (Sorry, on phone right now, can't look, perhaps it's fine)

I thought about it too, but not having a .help method is fine, and, it looks
that the only options which contains a .help method are those who have arguments
to the command itself. So, since inodes64 does not have arguments, a .help is
not necessary from what I could figure out.
> 
> Thanks,
> Eric
> 
> 
> > +
> >    add_command(&open_cmd);
> >    add_command(&stat_cmd);
> >    add_command(&close_cmd);
> > @@ -822,4 +860,5 @@ open_init(void)
> >    add_command(&chproj_cmd);
> >    add_command(&lsproj_cmd);
> >    add_command(&extsize_cmd);
> > +    add_command(&inodes64_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

-- 
Carlos

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

Re: [PATCH] xfs_io: Implement inodes64 command

[Carlos Maiolino]

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

Re: [PATCH] xfs_io: Implement inodes64 command

[Brian Foster]

On Tue, Sep 22, 2015 at 09:54:32AM +0200, Carlos Maiolino wrote:
> 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:
...
> > > +
> > > +	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?
> 

Not to speak for Dave, but I had the same thought just skimming through
the patch. I don't think it's a technical reason so much as a style one.
The further the function is grown, the easier it is to see that this
kind of flow is not the most friendly thing. E.g.:

	ret = do_stuff();
	if (!ret) {
		...
		ret = do_more_stuff();
		if (!ret) {
			...
		}
	}

In other words, the common execution path of the function starts to flow
"inward" rather than the natural top-down flow of the function:

	ret = do_stuff();
	if (ret)
		handle_err;

	ret = do_more_stuff();
	if (ret)
		handle_err;
	...

The latter tends to be easier to follow, easier to review the error
handling cases, probably avoids indentation problems down the road, etc.
The function as it is right now is simple and so this probably isn't a
big deal, but I suspect seeing the latter form so frequently in all of
our other code is probably what causes something like the former to
stand out (even in a simple/harmless example). Just my .02. :)

Brian

>  
> > > @@ -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

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

Re: [PATCH] xfs_io: Implement inodes64 command

[Dave Chinner]

On Tue, Sep 22, 2015 at 08:22:39AM -0400, Brian Foster wrote:
> On Tue, Sep 22, 2015 at 09:54:32AM +0200, Carlos Maiolino wrote:
> > 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:
> ...
> > > > +
> > > > +	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?
> > 
> 
> Not to speak for Dave, but I had the same thought just skimming through
> the patch. I don't think it's a technical reason so much as a style one.
> The further the function is grown, the easier it is to see that this
> kind of flow is not the most friendly thing. E.g.:
> 
> 	ret = do_stuff();
> 	if (!ret) {
> 		...
> 		ret = do_more_stuff();
> 		if (!ret) {
> 			...
> 		}
> 	}
> 
> In other words, the common execution path of the function starts to flow
> "inward" rather than the natural top-down flow of the function:
> 
> 	ret = do_stuff();
> 	if (ret)
> 		handle_err;
> 
> 	ret = do_more_stuff();
> 	if (ret)
> 		handle_err;
> 	...
> 
> The latter tends to be easier to follow, easier to review the error
> handling cases, probably avoids indentation problems down the road, etc.
> The function as it is right now is simple and so this probably isn't a
> big deal, but I suspect seeing the latter form so frequently in all of
> our other code is probably what causes something like the former to
> stand out (even in a simple/harmless example). Just my .02. :)

That's pretty much it from a code maintenance POV.

Also, though, consider how the compiler optimises code - the code
inside an if () condition usually involves resolving a branch
due to a jump statement to somewhere else. IOWs, your original code
might end up looking something like this:

	if (condition)
		goto L1
	<code>
ret:
	return
L1:
	if (condition)
		goto L2
	<code>
	goto ret;
L2:
	if (condition)
		goto L3
	<code>
	goto ret;
L3:
	.....

and so a series of nested conditions results in lots of branches
beign taken during the fast path execution. That leads to a 
larger CPU instruction cache footprint that the code generated from
the latter case, which looks like:

	if (!condition)
		goto L1
	if (!condition)
		goto L2
	if (!condition)
		goto L3
	<code>
ret:
	return
L1:
	<code>
	goto ret;
L2
	<code>
	goto ret;
L3:
	.....

See the difference in the fast path execution? It's compact and no
branches are taken anywhere and so has a smaller instruction cache
footprint. And less instruction cache misses mean faster code
execution.

Hence on top of the code is being easier to read, modify and
maintain, and it's also much easier for the compiler to optimise
into fast, efficient code....

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 inodes64 command - bug in XFS_IOC_FSINUMBERS?

[Carlos Maiolino]

Howdy folks,

I was working in implementing the suggested feature in my patch, about getting
the next inode used after one is provided, and I hit something that I'm not really
sure if this might be considered a bug, or just a work form.

XFS_IOC_FSINUMBERS, is supposed to be called with a zeroed
xfs_fsop_bulkreq.lastip, so at each call, kernel will update this number to the
last inode returned, and, the next call will return in xfs_inogrp.xi_startino,
the next existing inode after .lastip.

So, I was expecting that, passing a non-zero .lastip at the first call, I would
be able to get the next inode right after the one I passed through .lastip, but,
after some tests and reading the code, I noticed that this is not the case.

The problem (not sure if I can really say it's a problem), is that, if the inode
number passed, happens to be somewhere in the middle of an inode chunk, the
whole chunk will not be printed, only the next inode chunk will start to be
printed, hiding all information of the previous one.

I'm not sure if this is the desired behavior or not, but, I'd say that, if the
inode passed in .lastip, is not the first in the chunk, the output should start
for its own chunk, instead of the next one, but, I prefer to see you folks POV
before starting to fix something that I'm not sure if it's actually broken :-)

Cheers

-- 
Carlos

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

Re: [PATCH] xfs_io: Implement inodes64 command - bug in XFS_IOC_FSINUMBERS?

[Brian Foster]

On Wed, Sep 23, 2015 at 12:28:34PM +0200, Carlos Maiolino wrote:
> Howdy folks,
> 
> I was working in implementing the suggested feature in my patch, about getting
> the next inode used after one is provided, and I hit something that I'm not really
> sure if this might be considered a bug, or just a work form.
> 
> XFS_IOC_FSINUMBERS, is supposed to be called with a zeroed
> xfs_fsop_bulkreq.lastip, so at each call, kernel will update this number to the
> last inode returned, and, the next call will return in xfs_inogrp.xi_startino,
> the next existing inode after .lastip.
> 
> So, I was expecting that, passing a non-zero .lastip at the first call, I would
> be able to get the next inode right after the one I passed through .lastip, but,
> after some tests and reading the code, I noticed that this is not the case.
> 
> The problem (not sure if I can really say it's a problem), is that, if the inode
> number passed, happens to be somewhere in the middle of an inode chunk, the
> whole chunk will not be printed, only the next inode chunk will start to be
> printed, hiding all information of the previous one.
> 
> I'm not sure if this is the desired behavior or not, but, I'd say that, if the
> inode passed in .lastip, is not the first in the chunk, the output should start
> for its own chunk, instead of the next one, but, I prefer to see you folks POV
> before starting to fix something that I'm not sure if it's actually broken :-)
> 

On a quick pass through, it seems like this is probably just the nature
of the granularity and typical use case of the inumbers interface. For
one, I think the lastip parameter is intended to be used as a cookie as
opposed to an inode-granularity search key. The return structures also
appear to describe inode records so there isn't any natural way to get a
partial record that I can see. Note that bulkstat does appear to support
an inode granularity starting point (see xfs_bulkstat_grab_ichunk()) as
the output format is inode granularity.

FWIW, I would probably try to handle something like the above in
userspace by working with the interface rather than modifying it. In
other words, subtract (at least) XFS_INODES_PER_CHUNK from the inode
number in question and use that as a starting point to ensure the output
contains the record for the inode if one exists.

It seems like you could change the inumbers behavior to return the
record that contains lastip + 1 if you really wanted to, we'd just have
to take care not to break the lastip behavior itself (e.g., we can't
just search for any record that contains lastip). What looks
interesting, and might justify doing something in this area, is the
behavior presumably caused by doing a greater than or equal to
(XFS_LOOKUP_GE) lookup in xfs_inumbers(). If lastino happens to be a
record startino, we'll actually return the record that contains that
inode (thus technically supporting a mid-record start). If the index in
the record is beyond that, we'll skip it and start at the next record.
Unless I misinterpret that, it seems like that should be fixed one way
or another.

Brian

> Cheers
> 
> -- 
> Carlos
> 
> _______________________________________________
> 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 inodes64 command - bug in XFS_IOC_FSINUMBERS?

[Dave Chinner]

On Wed, Sep 23, 2015 at 12:28:34PM +0200, Carlos Maiolino wrote:
> Howdy folks,
> 
> I was working in implementing the suggested feature in my patch, about getting
> the next inode used after one is provided, and I hit something that I'm not really
> sure if this might be considered a bug, or just a work form.
> 
> XFS_IOC_FSINUMBERS, is supposed to be called with a zeroed
> xfs_fsop_bulkreq.lastip, so at each call, kernel will update this number to the
> last inode returned, and, the next call will return in xfs_inogrp.xi_startino,
> the next existing inode after .lastip.
> 
> So, I was expecting that, passing a non-zero .lastip at the first call, I would
> be able to get the next inode right after the one I passed through .lastip, but,
> after some tests and reading the code, I noticed that this is not the case.

XFS_IOC_FSNUMBERS is not a "does this inode exist" query API - you
use the bulkstat interface for that. XFS_IOC_FSNUMBERS is for
iterating the "inode table", and it's API returns records, not
individual inodes.

Those records contain information about a chunk of inodes, not
individual inodes. The "lastino" cookie it uses always points to the
last inode in the last chunk it returns - the next iteration will
start at the chunk *after* the one that contains lastino.

Hence it is behaving as intended...

> I'm not sure if this is the desired behavior or not, but, I'd say that, if the
> inode passed in .lastip, is not the first in the chunk, the output should start
> for its own chunk, instead of the next one, but, I prefer to see you folks POV
> before starting to fix something that I'm not sure if it's actually broken :-)

It doesn't matter if it is "desired behaviour" or not, we can't
change it. If we change it we risk breaking userspace applications
that relies on it working the way it currently does. Most likely
that application will be xfsdump, and the breakage will be silent
and very hard to detect....

Perhaps reading the recent history fs/xfs/xfs_itable.c would be
instructive. ;)

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 inodes64 command - bug in XFS_IOC_FSINUMBERS?

[Carlos Maiolino]

Thanks Brian and Eric,

I'll rework my patch according to this talk,


On Thu, Sep 24, 2015 at 09:10:40AM +1000, Dave Chinner wrote:
> On Wed, Sep 23, 2015 at 12:28:34PM +0200, Carlos Maiolino wrote:
> > Howdy folks,
> > 
> > I was working in implementing the suggested feature in my patch, about getting
> > the next inode used after one is provided, and I hit something that I'm not really
> > sure if this might be considered a bug, or just a work form.
> > 
> > XFS_IOC_FSINUMBERS, is supposed to be called with a zeroed
> > xfs_fsop_bulkreq.lastip, so at each call, kernel will update this number to the
> > last inode returned, and, the next call will return in xfs_inogrp.xi_startino,
> > the next existing inode after .lastip.
> > 
> > So, I was expecting that, passing a non-zero .lastip at the first call, I would
> > be able to get the next inode right after the one I passed through .lastip, but,
> > after some tests and reading the code, I noticed that this is not the case.
> 
> XFS_IOC_FSNUMBERS is not a "does this inode exist" query API - you
> use the bulkstat interface for that. XFS_IOC_FSNUMBERS is for
> iterating the "inode table", and it's API returns records, not
> individual inodes.
> 
> Those records contain information about a chunk of inodes, not
> individual inodes. The "lastino" cookie it uses always points to the
> last inode in the last chunk it returns - the next iteration will
> start at the chunk *after* the one that contains lastino.
> 
> Hence it is behaving as intended...
> 
> > I'm not sure if this is the desired behavior or not, but, I'd say that, if the
> > inode passed in .lastip, is not the first in the chunk, the output should start
> > for its own chunk, instead of the next one, but, I prefer to see you folks POV
> > before starting to fix something that I'm not sure if it's actually broken :-)
> 
> It doesn't matter if it is "desired behaviour" or not, we can't
> change it. If we change it we risk breaking userspace applications
> that relies on it working the way it currently does. Most likely
> that application will be xfsdump, and the breakage will be silent
> and very hard to detect....

I thought about this possibility too, but didn't mention in my e-mail, but, it's
good to know that.
> 
> Perhaps reading the recent history fs/xfs/xfs_itable.c would be
> instructive. ;)
> 
I certainly will :)


Cheers.

-- 
Carlos

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