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...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
