Hey Brian,
On Fri, Dec 07, 2012 at 03:49:56PM -0500, Brian Foster wrote:
> EDQUOT occurs in the buffered write path on user or group quota
> allocation failures (project quotas currently use ENOSPC). For
> inodes in multiple quotas, we do not know which quota might have
> led to the failure. We also cannot technically discern project
> quota ENOSPC from global ENOSPC. Therefore, we check the state of
> each quota applicable to the inode and run an eofblocks scan on
> each considered to be under limited free space conditions.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/xfs/xfs_file.c | 14 +++++++++++
> fs/xfs/xfs_icache.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_icache.h | 1 +
> 3 files changed, 81 insertions(+), 0 deletions(-)
>
> Hi guys,
>
> Here's an RFC that covers the recently discussed integration of eofblocks
> scanning into EDQUOT/ENOSPC error handling. This is incomplete and only spot
> tested against user quotas because I wanted to get some feedback on the approach
> before I proceed.
>
> My biggest question at this point is whether this is the best layer to include
> this behavior (i.e., in xfs_file_buffered_aio_write() where we've lost
> quota-related context on the error). An alternative approach I considered was to
> bury a scan/retry down in xfs_trans_dquot.c (i.e., in or around
> xfs_trans_reserve_quota_bydquots()), where the specific failure context is
> available. That would result in the higher level buffered write code remaining the
> same and only receiving an EDQUOT if we've already done the retry and failed. Any
> thoughts on that are appreciated.
>
> Brian
>
> TODOs:
> - I'm not a huge fan of the name of xfs_inode_free_quota_eofblocks(), but haven't
> thought of anything better yet. :)
> - Turn the quota checking into a range check (i.e., reserved blocks within %1 of
> the limit) and bury it in a 'xfs_quota_is_full()' macro somewhere.
> - Enhance eofblocks to support a flush operation and enable that in the project
> quota ENOSPC case.
> - It just occurred to me when looking this over I might need to use the
> XFS_IS_*QUOTA_ENFORCED macros around the quota space checks.
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 67284ed..505b9fb 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -35,6 +35,7 @@
> #include "xfs_dir2_priv.h"
> #include "xfs_ioctl.h"
> #include "xfs_trace.h"
> +#include "xfs_icache.h"
>
> #include <linux/dcache.h>
> #include <linux/falloc.h>
> @@ -716,6 +717,7 @@ xfs_file_buffered_aio_write(
> struct xfs_inode *ip = XFS_I(inode);
> ssize_t ret;
> int enospc = 0;
> + int edquot = 0;
> int iolock = XFS_IOLOCK_EXCL;
> size_t count = ocount;
>
> @@ -734,6 +736,18 @@ write_retry:
> pos, &iocb->ki_pos, count, 0);
>
> /*
> + * A quota failure can be represented as EDQUOT or ENOSPC in the case
> + * of project quotas. Check the quotas explicitly for low space
> + * conditions, run a prealloc scan if warranted and retry. Otherwise,
> + * proceed to general ENOSPC handling.
> + */
> + if ((ret == -EDQUOT || ret == -ENOSPC) && !edquot) {
> + edquot = 1;
> + if (xfs_inode_free_quota_eofblocks(ip))
> + goto write_retry;
> + }
It looks like you have the iolock held across this call to free eofblocks. Is
it possible that this inode would already have some eofblocks, be tagged on the
radix tree, and you'd try to get the lock a second time?
I think you're right about XFS_IS_*QUOTA_ENFORCED.
Neat patch! I wish we had a solid way to know which quota was enforced at this
level. Still, this is much better than trimming eofblocks on all inodes.
-Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
