[PATCH] xfs_quota: remove calls to XFS_QSYNC

[PATCH] xfs_quota: remove calls to XFS_QSYNC

[Christoph Hellwig]

In 2005 commit master-melb:xfs-cmds:23840a from Nathan added calls to
XFS_QSYNC/Q_XQUOTASYNC to xfs_quota, with the following rather sparse
description:

 "Issue a quote sync before reporting quota, resolving issue with delayed
  allocation."

I can't really see a reason for this - we do quota accounting by the time
we reserve space for the delayed allocation, and while converting the
reservations might change the quota accounting minimally due to the amount
of btree blocks used for the bmap btree on large files in generally this
makes little sense, and on today's large system has a large performance
impact.  Also only xfs_quota ever did these calls, the generic quota tool
never did any kind of sync, and of course removing it does not cause
any regressions in xfstests.

Nathan, I've cced you in case you still remember anything about this,
although it's fairly unlikely after 6.5 years.  Also if anyone at SGI
can find anything about the above commits in BugWorks additional feedback
would be welcome.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfsprogs-dev/quota/free.c
===================================================================
--- xfsprogs-dev.orig/quota/free.c	2012-01-27 11:10:34.000000000 +0000
+++ xfsprogs-dev/quota/free.c	2012-01-27 11:10:42.000000000 +0000
@@ -167,7 +167,6 @@ projects_free_space_data(
 		return 0;
 	}
 
-	xfsquotactl(XFS_QSYNC, dev, type, fsx.fsx_projid, NULL);
 	if (xfsquotactl(XFS_GETQUOTA, dev, type, fsx.fsx_projid, &d) < 0) {
 		perror("XFS_GETQUOTA");
 		close(fd);
Index: xfsprogs-dev/quota/irix.c
===================================================================
--- xfsprogs-dev.orig/quota/irix.c	2012-01-27 11:10:52.000000000 +0000
+++ xfsprogs-dev/quota/irix.c	2012-01-27 11:10:59.000000000 +0000
@@ -45,8 +45,6 @@ xcommand_to_qcommand(
 		return Q_XGETQSTAT;
 	case XFS_QUOTARM:
 		return Q_XQUOTARM;
-	case XFS_QSYNC:
-		return Q_SYNC;
 	}
 	return 0;
 }
Index: xfsprogs-dev/quota/linux.c
===================================================================
--- xfsprogs-dev.orig/quota/linux.c	2012-01-27 11:11:02.000000000 +0000
+++ xfsprogs-dev/quota/linux.c	2012-01-27 11:11:05.000000000 +0000
@@ -55,8 +55,6 @@ xcommand_to_qcommand(
 		return Q_XGETQSTAT;
 	case XFS_QUOTARM:
 		return Q_XQUOTARM;
-	case XFS_QSYNC:
-		return Q_XQUOTASYNC;
 	}
 	return 0;
 }
Index: xfsprogs-dev/quota/quota.c
===================================================================
--- xfsprogs-dev.orig/quota/quota.c	2012-01-27 11:10:24.000000000 +0000
+++ xfsprogs-dev/quota/quota.c	2012-01-27 11:10:29.000000000 +0000
@@ -64,7 +64,6 @@ quota_mount(
 	uint		qflags;
 	int		count;
 
-	xfsquotactl(XFS_QSYNC, dev, type, 0, NULL);
 	if (xfsquotactl(XFS_GETQUOTA, dev, type, id, (void *)&d) < 0)
 		return 0;
 
Index: xfsprogs-dev/quota/quota.h
===================================================================
--- xfsprogs-dev.orig/quota/quota.h	2012-01-27 11:11:10.000000000 +0000
+++ xfsprogs-dev/quota/quota.h	2012-01-27 11:11:20.000000000 +0000
@@ -41,7 +41,6 @@ enum {
 	XFS_SETQLIM,	/* set disk limits */
 	XFS_GETQSTAT,	/* get quota subsystem status */
 	XFS_QUOTARM,	/* free disk space used by dquots */
-	XFS_QSYNC,	/* flush delayed allocate space */
 };
 
 /*
Index: xfsprogs-dev/quota/report.c
===================================================================
--- xfsprogs-dev.orig/quota/report.c	2012-01-27 11:09:41.000000000 +0000
+++ xfsprogs-dev/quota/report.c	2012-01-27 11:10:19.000000000 +0000
@@ -520,10 +520,6 @@ report_any_type(
 	if (type & XFS_USER_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
-			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
-						XFS_USER_QUOTA, 0, NULL) < 0
-					&& errno != ENOENT && errno != ENOSYS)
-				perror("XFS_QSYNC user quota");
 			report_user_mount(fp, form, mount,
 						lower, upper, flags);
 		}
@@ -531,10 +527,6 @@ report_any_type(
 	if (type & XFS_GROUP_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
-			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
-						XFS_GROUP_QUOTA, 0, NULL) < 0
-					&& errno != ENOENT && errno != ENOSYS)
-				perror("XFS_QSYNC group quota");
 			report_group_mount(fp, form, mount,
 						lower, upper, flags);
 		}
@@ -542,10 +534,6 @@ report_any_type(
 	if (type & XFS_PROJ_QUOTA) {
 		fs_cursor_initialise(dir, FS_MOUNT_POINT, &cursor);
 		while ((mount = fs_cursor_next_entry(&cursor))) {
-			if (xfsquotactl(XFS_QSYNC, mount->fs_name,
-						XFS_PROJ_QUOTA, 0, NULL) < 0
-					&& errno != ENOENT && errno != ENOSYS)
-				perror("XFS_QSYNC proj quota");
 			report_project_mount(fp, form, mount,
 						lower, upper, flags);
 		}

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

Re: [PATCH] xfs_quota: remove calls to XFS_QSYNC

[Nathan Scott]

On 30 January 2012 22:50, Christoph Hellwig <hch@infradead.org> wrote:
> ...
> Nathan, I've cced you in case you still remember anything about this,
> although it's fairly unlikely after 6.5 years.  Also if anyone at SGI
> can find anything about the above commits in BugWorks additional feedback
> would be welcome.

I can't recall - but usually there was greater detail in the bugworks entry,
I think thats your best source now.

Patch looks OK to me FWIW, assuming nothing comes of the bugworks
archeology exercise.

cheers.

--
Nathan

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

Re: [PATCH] xfs_quota: remove calls to XFS_QSYNC

[Ben Myers]

On Tue, Jan 31, 2012 at 06:57:04AM +1100, Nathan Scott wrote:
> On 30 January 2012 22:50, Christoph Hellwig <hch@infradead.org> wrote:
> > ...
> > Nathan, I've cced you in case you still remember anything about this,
> > although it's fairly unlikely after 6.5 years.  Also if anyone at SGI
> > can find anything about the above commits in BugWorks additional feedback
> > would be welcome.
> 
> I can't recall - but usually there was greater detail in the bugworks entry,
> I think thats your best source now.
> 
> Patch looks OK to me FWIW, assuming nothing comes of the bugworks
> archeology exercise.

I'm not a ptools expert so I had to ask around.  This turned out to be

PV942815 - XFS quota reporting interacts badly with delalloc

"We get a constant trickle of confused people who've found that the used
space reported by repquota/quota does not get immediately updated after
creating new files / extending existing ones.  The problem is a result
of buffered IO in XFS doing delayed allocation - if we have not done an
allocation, the quota accounting has not been updated, so we end up with
effectively stale data being reported.

This affects CXFS too, of course.  After discussion a few weeks back on
the XFS conf. call, it was decided to not change CXFS (too expensive to
do a flush on all nodes) but that we can make some simple XFS changes to
make this less of a problem - i.e. flushing delalloc data just before we
extract quota information." -Nathan


Patch looks good to me.

Reviewed-by: Ben Myers <bpm@sgi.com>

-Ben

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

Re: [PATCH] xfs_quota: remove calls to XFS_QSYNC

[Dave Chinner]

On Tue, Jan 31, 2012 at 10:26:17AM -0600, Ben Myers wrote:
> On Tue, Jan 31, 2012 at 06:57:04AM +1100, Nathan Scott wrote:
> > On 30 January 2012 22:50, Christoph Hellwig <hch@infradead.org> wrote:
> > > ...
> > > Nathan, I've cced you in case you still remember anything about this,
> > > although it's fairly unlikely after 6.5 years.  Also if anyone at SGI
> > > can find anything about the above commits in BugWorks additional feedback
> > > would be welcome.
> > 
> > I can't recall - but usually there was greater detail in the bugworks entry,
> > I think thats your best source now.
> > 
> > Patch looks OK to me FWIW, assuming nothing comes of the bugworks
> > archeology exercise.
> 
> I'm not a ptools expert so I had to ask around.  This turned out to be
> 
> PV942815 - XFS quota reporting interacts badly with delalloc
> 
> "We get a constant trickle of confused people who've found that the used
> space reported by repquota/quota does not get immediately updated after
> creating new files / extending existing ones.  The problem is a result
> of buffered IO in XFS doing delayed allocation - if we have not done an
> allocation, the quota accounting has not been updated, so we end up with
> effectively stale data being reported.
> 
> This affects CXFS too, of course.  After discussion a few weeks back on
> the XFS conf. call, it was decided to not change CXFS (too expensive to
> do a flush on all nodes) but that we can make some simple XFS changes to
> make this less of a problem - i.e. flushing delalloc data just before we
> extract quota information." -Nathan

So effectively what that says to me is that quota only exports the
real block usage, even though it internally tracks delalloc
reservations. Perhaps an additionaly change to make in this case is
to fold the reserved blocks into what is reported to the quota
utilities?

Indeed, what is exported to userspace via xfs_qm_export_dquot() is
the information in the dquot core - the on-disk information - so
perhaps all we need to do is export dqp->q_res_bcount (the count of
real + reserved blocks) instead of the on-disk info?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_quota: remove calls to XFS_QSYNC

[Christoph Hellwig]

On Wed, Feb 01, 2012 at 08:01:10AM +1100, Dave Chinner wrote:
> So effectively what that says to me is that quota only exports the
> real block usage, even though it internally tracks delalloc
> reservations. Perhaps an additionaly change to make in this case is
> to fold the reserved blocks into what is reported to the quota
> utilities?
> 
> Indeed, what is exported to userspace via xfs_qm_export_dquot() is
> the information in the dquot core - the on-disk information - so
> perhaps all we need to do is export dqp->q_res_bcount (the count of
> real + reserved blocks) instead of the on-disk info?

That seems like a good idea, given that enforcement takes the
reservation into account.  To retain compatbility for the case of new
userspace and an old kernel I'd have to disable Q_XQUOTASYNC in the
kernel intead of in the tool, though.

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