xfs_fsr, performance related tweaks

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!

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

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

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

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.

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

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

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

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

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