xfs_fsr, performance related tweaks

xfs_fsr, performance related tweaks

[Just Marc]

Hi,

I'm using fsr extensively.

I noticed two things:

1. in xfs_fsr.c:
   if (fsx.fsx_xflags & XFS_XFLAG_NODEFRAG) {
       if (vflag)
           fsrprintf(_("%s: marked as don't defrag, ignoring\n"),
               fname);
       return(0);
   }

This check should be moved above the code that performs a stat on the 
file, to become the first check, this will help reduce redundant stat 
calls for heavily defragged fileystems.   A simple patch is attached for 
that.

2. Files for which 'No improvement will be made' should also be marked 
as no-defrag, this will avoid a ton of extra work in the future.   In my 
particular use case I have many hundreds of thousands of files on each 
volume (xfs_fsr runs in a never ending-loop as new files are constantly 
being added) and once the file is written to disk it is never changed 
again until deletion.   Optionally do this only when a special parameter 
is passed to fsr at command line? (that is, if you think this is not 
appropriate for all scenarios).  

I tried to accomplish this but it proved more difficult than I thought.  
While digging around in the fsr code, I didn't find out how fsr marks 
the file as no-defrag, I'm guessing this is done in kernel code via the 
ioctl that swaps the extents (xfs_swapext) ... Is that correct?

I looked at how the 'io' utility marks files as no-defrag; it calls 
xfsctl(path, fd, XFS_IOC_FSSETXATTR, &attr); -- this requires a path, 
which is not available to me when fsr is run in its default mode which 
traverses all xfs filesystems rather than gets to run on a single file.

Is there a way to extract the path in this case? Or possibly use another 
way to mark the inode as no-defrag without having to need the path -- 
just fd?  Of course this can be done by an external script which parses 
the output of xfs_fsr for these inodes, looks them up and marks them as 
such, but that's pretty messy and very inefficient.  I'd really like to 
do this as cleanly and efficiently as possible.

I would appreciate any feedback you have on this.    Please CC me as I'm 
not on this list.

Thanks!

xfs_fsr, performance related tweaks

[Just Marc]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

Hi,

I'm using fsr extensively.

I noticed two things:

1. in xfs_fsr.c:
   if (fsx.fsx_xflags & XFS_XFLAG_NODEFRAG) {
       if (vflag)
           fsrprintf(_("%s: marked as don't defrag, ignoring\n"),
               fname);
       return(0);
   }

This check should be moved above the code that performs a stat on the 
file, to become the first check, this will help reduce redundant stat 
calls for heavily defragged fileystems.   A simple patch is attached for 
that.

2. Files for which 'No improvement will be made' should also be marked 
as no-defrag, this will avoid a ton of extra work in the future.   In my 
particular use case I have many hundreds of thousands of files on each 
volume (xfs_fsr runs in a never ending-loop as new files are constantly 
being added) and once the file is written to disk it is never changed 
again until deletion.   Optionally do this only when a special parameter 
is passed to fsr at command line? (that is, if you think this is not 
appropriate for all scenarios).  

I tried to accomplish this but it proved more difficult than I thought.  
While digging around in the fsr code, I didn't find out how fsr marks 
the file as no-defrag, I'm guessing this is done in kernel code via the 
ioctl that swaps the extents (xfs_swapext) ... Is that correct?

I looked at how the 'io' utility marks files as no-defrag; it calls 
xfsctl(path, fd, XFS_IOC_FSSETXATTR, &attr); -- this requires a path, 
which is not available to me when fsr is run in its default mode which 
traverses all xfs filesystems rather than gets to run on a single file.

Is there a way to extract the path in this case? Or possibly use another 
way to mark the inode as no-defrag without having to need the path -- 
just fd?  Of course this can be done by an external script which parses 
the output of xfs_fsr for these inodes, looks them up and marks them as 
such, but that's pretty messy and very inefficient.  I'd really like to 
do this as cleanly and efficiently as possible.

I would appreciate any feedback you have on this.    Please CC me as I'm 
not on this list.

Thanks!

[-- Attachment #2: xfs_fsr.diff --]
[-- Type: text/plain, Size: 1370 bytes --]

--- xfs_fsr.c	2007-06-28 13:23:58.745778214 +0100
+++ xfs_fsr.c.orig	2007-06-28 07:40:42.572069164 +0100
@@ -904,6 +904,20 @@
 		}
 	}
 
+	/* Check if there is room to copy the file */
+	if ( statvfs64( (fsname == NULL ? fname : fsname), &vfss) < 0) {
+		fsrprintf(_("unable to get fs stat on %s: %s\n"),
+			fname, strerror(errno));
+		return (-1);
+	}
+	bsize = vfss.f_frsize ? vfss.f_frsize : vfss.f_bsize;
+
+	if (statp->bs_size > ((vfss.f_bfree * bsize) - minimumfree)) {
+		fsrprintf(_("insufficient freespace for: %s: "
+			"size=%lld: ignoring\n"), fname, statp->bs_size);
+		return 1;
+	}
+
 	if ((ioctl(fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
 		fsrprintf(_("failed to get inode attrs: %s\n"), fname);
 		return(-1);
@@ -937,20 +951,6 @@
 		return -1;
 	}
 
-	/* Check if there is room to copy the file */
-	if ( statvfs64( (fsname == NULL ? fname : fsname), &vfss) < 0) {
-		fsrprintf(_("unable to get fs stat on %s: %s\n"),
-			fname, strerror(errno));
-		return (-1);
-	}
-	bsize = vfss.f_frsize ? vfss.f_frsize : vfss.f_bsize;
-
-	if (statp->bs_size > ((vfss.f_bfree * bsize) - minimumfree)) {
-		fsrprintf(_("insufficient freespace for: %s: "
-			"size=%lld: ignoring\n"), fname, statp->bs_size);
-		return 1;
-	}
-
 	/*
 	 * Previously the code forked here, & the child changed it's uid to
 	 * that of the file's owner and then called packfile(), to keep

Re: xfs_fsr, performance related tweaks

[Eric Sandeen]

Just Marc wrote:

> 2. Files for which 'No improvement will be made' should also be marked 
> as no-defrag, this will avoid a ton of extra work in the future.   

But... that file could drastically change in the future, no?  Just
because it can't be improved now doesn't necessarily mean that it should
never be revisited on subsequent runs, does it?

-Eric

Re: xfs_fsr, performance related tweaks

[Nathan Scott]

On Thu, 2007-06-28 at 13:47 +0100, Just Marc wrote:
> ...
> I looked at how the 'io' utility marks files as no-defrag; it calls 
> xfsctl(path, fd, XFS_IOC_FSSETXATTR, &attr); -- this requires a path, 
> which is not available to me when fsr is run in its default mode
> which 
> traverses all xfs filesystems rather than gets to run on a single
> file. 

Just call the ioctl directly - fsr is already doing this in a bunch
of places (even has a call to XFS_IOC_FSSETXATTR already, elsewhere).
The xfsctl wrapper is just to give some tools platform independence -
on IRIX (shares xfs_io code) some of the syscalls take paths, but on
Linux only file descriptors are used.

cheers.

--
Nathan

Re: xfs_fsr, performance related tweaks

[Andi Kleen]

Eric Sandeen <sandeen@sandeen.net> writes:

> Just Marc wrote:
> 
> > 2. Files for which 'No improvement will be made' should also be marked 
> > as no-defrag, this will avoid a ton of extra work in the future.   
> 
> But... that file could drastically change in the future, no?  Just
> because it can't be improved now doesn't necessarily mean that it should
> never be revisited on subsequent runs, does it?

I guess one could define an additional dont-defrag (or perhaps
rather already-defrag) flag that is always
cleared when the file changes. That could be safely set here.

But then I'm not sure it would be worth the effort. Why would you
run fsr that often that it matters?

Also I would expect that one can easily detect in many cases an defragmented
file by looking at the number of extents in the inode only and that would 
make it equivalent to the flag. The cases where this is not the case
are probably rare too.

-Andi

Re: xfs_fsr, performance related tweaks

[Just Marc]

Hi Eric,

In my particular case, and I'm sure for many other people, files that 
are stored never change again until they deleted.  I hinted that there 
could be a command line switch to turn this functionality on, as it may 
not be perfect for everyone's use cases.

If nobody likes this still, I would appreciate some hints on how to mark 
files as no-defrag from within fsr itself given that I only have the 
file descriptor ... A hack like looking up the descriptor in 
/proc/self/fd should work, but is linux specific and is too hackish in 
my opinion.   I'd like to at least have a nice simple patch for my own uses.

Marc


Eric Sandeen wrote:
> Just Marc wrote:
>
>   
>> 2. Files for which 'No improvement will be made' should also be marked 
>> as no-defrag, this will avoid a ton of extra work in the future.   
>>     
>
> But... that file could drastically change in the future, no?  Just
> because it can't be improved now doesn't necessarily mean that it should
> never be revisited on subsequent runs, does it?
>
> -Eric
>
>
>   

Re: xfs_fsr, performance related tweaks

[Just Marc]

Hi Nathan, Andy,

I tried calling the ioctl of course, it does accept a path (also in the 
example in which it is used) but it returned EINVAL, I'll try again.

 > I guess one could define an additional dont-defrag (or perhaps
 > rather already-defrag) flag that is always
 > cleared when the file changes. That could be safely set here.

I had this in mind but thought not to bring it up as it's too low 
level.  Although I prefer this solution myself as it caters for all 
cases automatically.

 > But then I'm not sure it would be worth the effort. Why would you run 
fsr that often that it matters?

I run fsr all the time because in my case there is hundreds of gigs of 
new data added to each file system every day, some of it does badly need 
to be defragged as the files added are actively being served, not just 
stored.

 >Also I would expect that one can easily detect in many cases an 
defragmented file by looking at the number of extents in the inode only 
and that would
 > make it equivalent to the flag. The cases where this is not the case 
are probably rare too.

Well, this case would change from file to file (depending on its size) 
and a high number of extents may be acceptable for very large files so 
either come up with a formula that says "I can accept X extents per gig 
and not continue defragging" or do clear the no-defrag bit on file 
modification which is a cleaner solution.

Marc


Nathan Scott wrote:
> Just call the ioctl directly - fsr is already doing this in a bunch
> of places (even has a call to XFS_IOC_FSSETXATTR already, elsewhere).
> The xfsctl wrapper is just to give some tools platform independence -
> on IRIX (shares xfs_io code) some of the syscalls take paths, but on
> Linux only file descriptors are used.
>
> cheers.
>
> --
> Nathan
>
>
>   

Re: xfs_fsr, performance related tweaks

[Just Marc]

Barry Naujok wrote:
> On Fri, 29 Jun 2007 16:21:58 +1000, Just Marc <marc@corky.net> wrote:
>
>> Hi Eric,
>>
>> In my particular case, and I'm sure for many other people, files that 
>> are stored never change again until they deleted.  I hinted that 
>> there could be a command line switch to turn this functionality on, 
>> as it may not be perfect for everyone's use cases.
>>
>> If nobody likes this still, I would appreciate some hints on how to 
>> mark files as no-defrag from within fsr itself given that I only have 
>> the file descriptor ... A hack like looking up the descriptor in 
>> /proc/self/fd should work, but is linux specific and is too hackish 
>> in my opinion.   I'd like to at least have a nice simple patch for my 
>> own uses.
>
> Eric,
>
> You can use the xfs_io chattr command to mark known files as
> nodefrag. Using the chattr -R option can be used to recurse
> directories.
>
>

Barry,

That's right but I can't do this on a filesystem that's just been 
defragged say a minute ago, and in the mean time 20 new files got added 
(I don't know what these files are... ).

The whole filesystem has to be scanned again and here comes the issue: 
fsr wants to try and reduce the extents of many files for which it can't 
do it thus incurring lots of work and load until it can even reach the 
new files do defrag them...

Marc

Re: xfs_fsr, performance related tweaks

[Barry Naujok]

On Fri, 29 Jun 2007 16:21:58 +1000, Just Marc <marc@corky.net> wrote:

> Hi Eric,
>
> In my particular case, and I'm sure for many other people, files that  
> are stored never change again until they deleted.  I hinted that there  
> could be a command line switch to turn this functionality on, as it may  
> not be perfect for everyone's use cases.
>
> If nobody likes this still, I would appreciate some hints on how to mark  
> files as no-defrag from within fsr itself given that I only have the  
> file descriptor ... A hack like looking up the descriptor in  
> /proc/self/fd should work, but is linux specific and is too hackish in  
> my opinion.   I'd like to at least have a nice simple patch for my own  
> uses.

Eric,

You can use the xfs_io chattr command to mark known files as
nodefrag. Using the chattr -R option can be used to recurse
directories.

Regards,
Barry.

Re: xfs_fsr, performance related tweaks

[David Chinner]

On Fri, Jun 29, 2007 at 07:41:15AM +0100, Just Marc wrote:
> Barry Naujok wrote:
> >You can use the xfs_io chattr command to mark known files as
> >nodefrag. Using the chattr -R option can be used to recurse
> >directories.
> 
> That's right but I can't do this on a filesystem that's just been 
> defragged say a minute ago, and in the mean time 20 new files got added 
> (I don't know what these files are... ).

So walk the filesystem with a script that queries the number of
extents in each file, and if they have a single extent then
run the xfs_io on them.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: xfs_fsr, performance related tweaks

[Just Marc]

David,

In my first post I already said something like that can be done but it's 
just an ugly hack.   Don't you think it would best be handled cleanly 
and correctly by fsr itself?



David Chinner wrote:
> On Fri, Jun 29, 2007 at 07:41:15AM +0100, Just Marc wrote:
>   
>> Barry Naujok wrote:
>>     
>>> You can use the xfs_io chattr command to mark known files as
>>> nodefrag. Using the chattr -R option can be used to recurse
>>> directories.
>>>       
>> That's right but I can't do this on a filesystem that's just been 
>> defragged say a minute ago, and in the mean time 20 new files got added 
>> (I don't know what these files are... ).
>>     
>
> So walk the filesystem with a script that queries the number of
> extents in each file, and if they have a single extent then
> run the xfs_io on them.
>
> Cheers,
>
> Dave.
>   

Re: xfs_fsr, performance related tweaks

[Nathan Scott]

On Fri, 2007-06-29 at 08:16 +0100, Just Marc wrote:
> 
> 
> In my first post I already said something like that can be done but
> it's 
> just an ugly hack.   Don't you think it would best be handled cleanly 
> and correctly by fsr itself?
> 

As I said earlier, fsr already issues the ioctl you're
concerned about using - I'm not sure what the issue is
there - if you need to do a setxattr, Just Do It.

cheers.

--
Nathan

Re: xfs_fsr, performance related tweaks

[Just Marc]

I agree with you.  But what about files it works on, heavily, then 
decides they can't be defragged further? then it tries to defrag them 
again and again and again.  And that's the end of my story.


David Chinner wrote:
> No, I don't - if you want files not to be defragmented, then you
> have to set the flags yourself in some way. You have a specific need
> that can be solved by some scripting to describe your defrag/no
> defrag policy. xfs_fsr has no place is setting defrag policy; it's
> function is simply to find and defrag files.
>
> Cheers,
>
> Dave.
>   

Re: xfs_fsr, performance related tweaks

[David Chinner]

On Fri, Jun 29, 2007 at 08:16:28AM +0100, Just Marc wrote:
> David,
> 
> In my first post I already said something like that can be done but it's 
> just an ugly hack.   Don't you think it would best be handled cleanly 
> and correctly by fsr itself?

No, I don't - if you want files not to be defragmented, then you
have to set the flags yourself in some way. You have a specific need
that can be solved by some scripting to describe your defrag/no
defrag policy. xfs_fsr has no place is setting defrag policy; it's
function is simply to find and defrag files.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: xfs_fsr, performance related tweaks

[Andi Kleen]

> I run fsr all the time because in my case there is hundreds of gigs of 
> new data added to each file system every day, some of it does badly need 
> to be defragged as the files added are actively being served, not just 

It might be better to investigate why XFS does such a poor job
for your workload in the first case. Unless the file systems
are always nearly full or you have a lot of holes it shouldn't fragment
that badly in the first place.

-Andi

Re: xfs_fsr, performance related tweaks

[Just Marc]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

Many files are being added concurrently, 24/7.   You might have hit the 
nail on the head, some of the files it was not able to improve are on 
filesystems that are almost full.    As the patch is done anyway here it is.

It does three things:

1. Actually make the usage printing become visible using -v,  the 
default case of getopt was never reached.
2. Reduces the number of 'stat' calls when scanning a filesystem for 
work to do, it now first checks if the file is marked as no-defrag 
before stat'ing it
3. Optionally, the -X parameter will tell fsr not to defrag files which 
it had decided it can't improve: 'No improvement will be made' ...

Marc


Andi Kleen wrote:
>> I run fsr all the time because in my case there is hundreds of gigs of 
>> new data added to each file system every day, some of it does badly need 
>> to be defragged as the files added are actively being served, not just 
>>     
>
> It might be better to investigate why XFS does such a poor job
> for your workload in the first case. Unless the file systems
> are always nearly full or you have a lot of holes it shouldn't fragment
> that badly in the first place.
>
> -Andi
>
>   


[-- Attachment #2: xfs_fsr.diff --]
[-- Type: text/plain, Size: 3278 bytes --]

--- xfs_fsr.c.orig	2007-06-28 07:40:42.572069164 +0100
+++ xfs_fsr.c	2007-06-29 09:18:44.330906899 +0100
@@ -68,6 +68,7 @@
 
 int vflag;
 int gflag;
+int xflag;
 static int Mflag;
 /* static int nflag; */
 int dflag = 0;
@@ -218,7 +219,7 @@
 
 	gflag = ! isatty(0);
 
-	while ((c = getopt(argc, argv, "C:p:e:MgsdnvTt:f:m:b:N:FV")) != -1 )
+	while ((c = getopt(argc, argv, "C:p:e:MgsdnvTt:f:m:b:N:FVXh")) != -1 )
 		switch (c) {
 		case 'M':
 			Mflag = 1;
@@ -267,7 +268,10 @@
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
-		default:
+		case 'X':
+			 xflag = 1; /* no eXtra work */
+			 break;
+		case 'h':
 			usage(1);
 		}
 	if (vflag)
@@ -371,6 +375,8 @@
 "       -f leftoff      Use this instead of /etc/fsrlast.\n"
 "       -m mtab         Use something other than /etc/mtab.\n"
 "       -d              Debug, print even more.\n"
+"       -h              Show usage.\n"
+"       -X              Mark as no-defrag files which can't be defragged further.\n"
 "       -v		Verbose, more -v's more verbose.\n"
 		), progname, progname);
 	exit(ret);
@@ -904,20 +910,6 @@
 		}
 	}
 
-	/* Check if there is room to copy the file */
-	if ( statvfs64( (fsname == NULL ? fname : fsname), &vfss) < 0) {
-		fsrprintf(_("unable to get fs stat on %s: %s\n"),
-			fname, strerror(errno));
-		return (-1);
-	}
-	bsize = vfss.f_frsize ? vfss.f_frsize : vfss.f_bsize;
-
-	if (statp->bs_size > ((vfss.f_bfree * bsize) - minimumfree)) {
-		fsrprintf(_("insufficient freespace for: %s: "
-			"size=%lld: ignoring\n"), fname, statp->bs_size);
-		return 1;
-	}
-
 	if ((ioctl(fd, XFS_IOC_FSGETXATTR, &fsx)) < 0) {
 		fsrprintf(_("failed to get inode attrs: %s\n"), fname);
 		return(-1);
@@ -951,6 +943,20 @@
 		return -1;
 	}
 
+	/* Check if there is room to copy the file */
+	if ( statvfs64( (fsname == NULL ? fname : fsname), &vfss) < 0) {
+		fsrprintf(_("unable to get fs stat on %s: %s\n"),
+			fname, strerror(errno));
+		return (-1);
+	}
+	bsize = vfss.f_frsize ? vfss.f_frsize : vfss.f_bsize;
+
+	if (statp->bs_size > ((vfss.f_bfree * bsize) - minimumfree)) {
+		fsrprintf(_("insufficient freespace for: %s: "
+			"size=%lld: ignoring\n"), fname, statp->bs_size);
+		return 1;
+	}
+
 	/*
 	 * Previously the code forked here, & the child changed it's uid to
 	 * that of the file's owner and then called packfile(), to keep
@@ -1128,11 +1134,32 @@
 	if (dflag)
 		fsrprintf(_("Temporary file has %d extents (%d in original)\n"), new_nextents, cur_nextents);
 	if (cur_nextents <= new_nextents) {
+	 	struct fsxattr fsx_tmp;	
+
 		if (vflag)
 			fsrprintf(_("No improvement will be made (skipping): %s\n"), fname);
 		free(fbuf);
+
+	 	if (xflag) {
+			/* Get the current inode flags */
+			if ((ioctl(fd, XFS_IOC_FSGETXATTR, &fsx_tmp)) < 0) {
+				fsrprintf(_("failed to get inode attrs: %s\n"), fname);
+				return -1;
+			}
+		
+			/* Add no-defrag */
+			fsx_tmp.fsx_xflags |= XFS_XFLAG_NODEFRAG;
+
+			/* Mark it! */
+			if (ioctl(fd, XFS_IOC_FSSETXATTR, &fsx_tmp) < 0) {
+				fsrprintf(_("could not set inode attrs on: %s\n"), fname);
+				close(tfd);
+				return -1;
+			}
+		}
+	
 		close(tfd);
-		return 1; /* no change/no error */
+		return 0; /* We're done with this file, forever. */
 	}
 
 	/* Loop through block map copying the file. */

Re: xfs_fsr, performance related tweaks

[Andi Kleen]

On Fri, Jun 29, 2007 at 09:23:04AM +0100, Just Marc wrote:

> You might have hit the 
> nail on the head, some of the files it was not able to improve are on 
> filesystems that are almost full.    

Don't do that then. It's probably the reason you get the bad fragmentation
in the first place. Most file systems perform very poorly when
they are nearly full.

-Andi

Re: xfs_fsr, performance related tweaks

[Eric Sandeen]

David Chinner wrote:
> On Fri, Jun 29, 2007 at 08:16:28AM +0100, Just Marc wrote:
>> David,
>>
>> In my first post I already said something like that can be done but it's 
>> just an ugly hack.   Don't you think it would best be handled cleanly 
>> and correctly by fsr itself?
> 
> No, I don't - if you want files not to be defragmented, then you
> have to set the flags yourself in some way. You have a specific need
> that can be solved by some scripting to describe your defrag/no
> defrag policy. xfs_fsr has no place is setting defrag policy; it's
> function is simply to find and defrag files.

I wouldn't mind seeing a way to tell fsr to not worry about defragging
some files based on current layout; say if the avg extent in the file is
> 100MB, or > 1G, don't bother... if today you have a 4.7G DVD iso image
in 3 extents (not bad) fsr will try to "fix" it for you right?

-eric


> Cheers,
> 
> Dave.

Re: xfs_fsr, performance related tweaks

[David Chinner]

On Sat, Jun 30, 2007 at 01:17:29PM -0400, Eric Sandeen wrote:
> David Chinner wrote:
> > On Fri, Jun 29, 2007 at 08:16:28AM +0100, Just Marc wrote:
> >> David,
> >>
> >> In my first post I already said something like that can be done but it's 
> >> just an ugly hack.   Don't you think it would best be handled cleanly 
> >> and correctly by fsr itself?
> > 
> > No, I don't - if you want files not to be defragmented, then you
> > have to set the flags yourself in some way. You have a specific need
> > that can be solved by some scripting to describe your defrag/no
> > defrag policy. xfs_fsr has no place is setting defrag policy; it's
> > function is simply to find and defrag files.
> 
> I wouldn't mind seeing a way to tell fsr to not worry about defragging
> some files based on current layout; say if the avg extent in the file is
> > 100MB, or > 1G, don't bother... if today you have a 4.7G DVD iso image
> in 3 extents (not bad) fsr will try to "fix" it for you right?

That could be easily done with command line options, I think. Define
the minimum extent length or number of extents we want files to have
and ignore those that are outside that criteria.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group