xfs
[Top] [All Lists]

Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 28 May 2014 08:42:58 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140527211416.GB6677@dastard>
References: <1400845950-41435-1-git-send-email-bfoster@xxxxxxxxxx> <1400845950-41435-3-git-send-email-bfoster@xxxxxxxxxx> <20140526225755.GR18954@dastard> <20140527124755.GC63281@xxxxxxxxxxxxxxx> <20140527211416.GB6677@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, May 28, 2014 at 07:14:16AM +1000, Dave Chinner wrote:
> On Tue, May 27, 2014 at 08:47:55AM -0400, Brian Foster wrote:
> > On Tue, May 27, 2014 at 08:57:55AM +1000, Dave Chinner wrote:
> > > On Fri, May 23, 2014 at 07:52:29AM -0400, Brian Foster wrote:
> > > > +/*
> > > > + * Run eofblocks scans on the quotas applicable to the inode. For 
> > > > inodes with
> > > > + * multiple quotas, we don't know exactly which quota caused an 
> > > > allocation
> > > > + * failure. We make a best effort by running scans for each quota 
> > > > considered
> > > > + * to be under low free space conditions (less than 1% available free 
> > > > space).
> > > > + */
> > > > +int
> > > > +xfs_inode_free_quota_eofblocks(
> > > > +       struct xfs_inode *ip)
> > > > +{
> > > > +       int scanned = 0;
> > > > +       struct xfs_eofblocks eofb = {0,};
> > > > +       struct xfs_dquot *dq;
> > > > +
> > > > +       ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> > > > +
> > > > +       /* set the scan owner to avoid potential livelock */
> > > > +       eofb.eof_scan_owner = ip->i_ino;
> > > > +
> > > > +       if (XFS_IS_UQUOTA_ENFORCED(ip->i_mount)) {
> > > > +               dq = xfs_inode_dquot(ip, XFS_DQ_USER);
> > > > +               if (dq && xfs_dquot_lowsp(dq)) {
> > > > +                       eofb.eof_uid = VFS_I(ip)->i_uid;
> > > > +                       eofb.eof_flags = XFS_EOF_FLAGS_SYNC|
> > > > +                                        XFS_EOF_FLAGS_UID;
> > > > +                       xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> > > > +                       scanned = 1;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       if (XFS_IS_GQUOTA_ENFORCED(ip->i_mount)) {
> > > > +               dq = xfs_inode_dquot(ip, XFS_DQ_GROUP);
> > > > +               if (dq && xfs_dquot_lowsp(dq)) {
> > > > +                       eofb.eof_gid = VFS_I(ip)->i_gid;
> > > > +                       eofb.eof_flags = XFS_EOF_FLAGS_SYNC|
> > > > +                                        XFS_EOF_FLAGS_GID;
> > > > +                       xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> > > > +                       scanned = 1;
> > > > +               }
> > > > +       }
> > > 
> > > Rather that doing two scans here, wouldn't it be more efficient
> > > to do:
> > > 
> > >   eofb.eof_flags = XFS_EOF_FLAGS_SYNC;
> > >   scan = false;
> > >   if (uquota is low) {
> > >           eofb.eof_uid = VFS_I(ip)->i_uid;
> > >           eofb.eof_flags |= XFS_EOF_FLAGS_UID;
> > >           scan = true;
> > >   }
> > >   if (gquota is low) {
> > >           eofb.eof_gid = VFS_I(ip)->i_gid;
> > >           eofb.eof_flags |= XFS_EOF_FLAGS_GID;
> > >           scan = true;
> > >   }
> > >   if (scan)
> > >           xfs_icache_free_eofblocks(ip->i_mount, &eofb);
> > > 
> > > and change xfs_inode_match_id() to be able to check against multiple
> > > flags on a single inode? That way we only scan the inode cache
> > > once, regardless of the number of quota types that are enabled and
> > > are tracking low space thresholds.
> > > 
> > 
> > Yeah, that would certainly be better from this perspective. We don't
> > care so much about the characteristics of the inode as much as the
> > quotas that are associated with it. If I recall, I was somewhat on the
> > fence about this behavior when we first added the userspace interface
> > here. IOWs, should the combination of flags define an intersection of
> > the set of inodes to scan or a union? The more I think about it, I think
> > the interface kind of suggests the former (from an interface/aesthetic
> > perspective). E.g., I probably wouldn't expect to add a GID flag to a
> > UID flag and have my scan become more broad, rather than more
> > restrictive. Otherwise, the existence of a uid, gid and prid in the
> > request structure seems kind of arbitrary (as opposed to a list/set of
> > generic IDs, for example).
> > 
> > I'm not against union behavior in general (and still probably not 100%
> > against switching the default). I suppose another option could be to add
> > a set of union/intersection flags that control the behavior here. I'd
> > be slightly concerned about making this interface too convoluted, but it
> > is a relatively low level thing, I suppose, without much generic use. We
> > could also decide not to expose those extra controls to userspace for
> > the time being.
> > 
> > I need to think about this some more. Thoughts on any of that?
> 
> What we expose to userspace is orthoganol to what we implment
> internally. It makes sense to limit the userspace interface to a
> single type at a time, but when we are doing internal cleaner work
> it makes sense to match all criteria in a single cache pass.
> 
> i.e. Restrict the capability of the user interface at the input
> layer rather than restricting the capability of the infrastructure
> to do work efficiently...
> 

Ok... so I'm thinking we can handle this with a new XFS_EOF_FLAGS_UNION
flag. This is masked off from userspace requests. It will be set for the
internal quota scan and xfs_inode_match_id() can check for it and call a
*match_id_union() variant that does the right thing. Thanks.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>