xfs: run eofblocks scan on ENOSPC
v2 cover xfs: run eofblocks scan on ENOSPC
Hi all,
Here's v2 of the eofblocks scan on ENOSPC series, incorporating feedback from v1:
http://oss.sgi.com/archives/xfs/2014-03/msg00388.html
The major change here is to simplify the error checking logic and tie the eofblocks scan to the inode flush in the ENOSPC scenario. I've done some high-level testing that doesn't seem to elicit any sort of pathological behavior given the circumstances (i.e., performance will never be ideal as we head into ENOSPC).
I tested on a hacked filesystem that makes preallocation persistent (no trim on close), disables preallocation throttling and set the background scanner to a high value to create worst case conditions. I ran an fs_mark workload to create 64k 1MB files. Then, started 16x8GB sequential dd writers expected to hit ENOSPC. This is on a 16xcpu box with 32GB RAM and a 200GB fs (with agcounts of 32 and 1024).
Via tracepoints, I generally observe that the inode flush acts as a filter to prevent many threads from entering into eofblocks scans at once. E.g., by the time the first handful of threads make it through a scan, they and/or others have dirtied more data for the remaining queued up inode flushers to work with. I notice some occasional spikes in kworkers or rcu processing, but nothing for longer than a couple seconds or so.
A downside I've noticed with this logic is that once one thread runs a scan and makes it through this retry sequence, it has a better chance to allocate more of the recently freed space than the others, all of which might have queued on the inode flush lock by the time the first flush/scan completes.
This leads to what one might consider "unfair" allocation across the set of writers when we enter this scenario. E.g., I saw tests were some threads were able to complete the 8GB write while others only made it to 2-3GB before the filesystem completely ran out of space. Given the benefit of the series, I think this is something that can be potentially enhanced incrementally if it turns out to be a problem in practice.
I also have an xfstests test I'm planning to post soon that verifies lingering preallocations can be reclaimed in a reasonable manner before returning ENOSPC.
Thoughts, reviews, flames appreciated.
Brian
v2:
- Drop flush mechanism during eofblocks scan (along with prereq patch).
- Simplify scan logic on ENOSPC. Separate EDQUOT from ENOSPC and tie ENOSPC scan to inode flush.
- Eliminate unnecessary project quota handling from xfs_inode_free_quota_eofblocks() (ENOSPC is a separate path).
Brian Foster (3):
xfs: add scan owner field to xfs_eofblocks
xfs: run an eofblocks scan on ENOSPC/EDQUOT
xfs: squash prealloc while over quota free space as well
fs/xfs/xfs_dquot.h | 15 ++++++++++++++ fs/xfs/xfs_file.c | 23 +++++++++++++++++---- fs/xfs/xfs_icache.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++- fs/xfs/xfs_icache.h | 3 +++ fs/xfs/xfs_iomap.c | 20 ++++++++++++------ 5 files changed, 109 insertions(+), 11 deletions(-)
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/3 xfs: add scan owner field to xfs_eofblocks
The scan owner field represents an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eofblocks. This is an internal only field.
--- fs/xfs/xfs_icache.c | 12 +++++++++++- fs/xfs/xfs_icache.h | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-)
fs/xfs/xfs_icache.c +11 -1
@@ -1211,6 +1211,7 @@ xfs_inode_free_eofblocks( {int ret;struct xfs_eofblocks *eofb = args; +bool need_iolock = true;if (!xfs_can_free_eofblocks (ip, false)) {/* inode could be preallocated or append-only */
@@ -1235,9 +1236,18 @@ xfs_inode_free_eofblocks(if (eofb->eof_flags &XFS_EOF_FLAGS_MINFILESIZE &&XFS_ISIZE (ip) < eofb->eof_min_file_size)return 0 ; + +/* +* A scan owner implies we already hold the iolock. Skip it in +* xfs_free_eofblocks() to avoid deadlock. This also eliminates +* the possibility of EAGAIN being returned. +*/ +if (eofb->eof_scan_owner !=NULLFSINO && + eofb->eof_scan_owner == ip->i_ino) + need_iolock = false; } - ret =xfs_free_eofblocks (ip->i_mount, ip, true); + ret =xfs_free_eofblocks (ip->i_mount, ip, need_iolock);/* don't revisit the inode if we're not waiting */ if (ret ==EAGAIN && !(flags &SYNC_WAIT ))
fs/xfs/xfs_icache.h +2 -0
@@ -27,6 +27,7 @@ struct xfs_eofblocks {kgid_t eof_gid;prid_t eof_prid;__u64 eof_min_file_size; +xfs_ino_t eof_scan_owner; };#define SYNC_WAIT 0x0001/* wait for i/o to complete */
@@ -84,6 +85,7 @@ xfs_fs_eofblocks_from_user( dst->eof_flags = src->eof_flags; dst->eof_prid = src->eof_prid; dst->eof_min_file_size = src->eof_min_file_size; + dst->eof_scan_owner =NULLFSINO ; dst->eof_uid =INVALID_UID ;if (src->eof_flags &XFS_EOF_FLAGS_UID ) {
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
Quoted 26 lines · Brian Foster
The scan owner field represents an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eofblocks. This is an internal only field.
Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_icache.c | 12 +++++++++++- fs/xfs/xfs_icache.h | 2 ++ 2 files changed, 13 insertions(+), 1 deletion(-)fs/xfs/xfs_icache.c +1
-0@@ -1211,6 +1211,7 @@ xfs_inode_free_eofblocks( {int ret;struct xfs_eofblocks *eofb = args; +bool need_iolock = true;if (!xfs_can_free_eofblocks (ip, false)) {/* inode could be preallocated or append-only */ @@ -1235,9 +1236,18 @@ xfs_inode_free_eofblocks(if (eofb->eof_flags &XFS_EOF_FLAGS_MINFILESIZE &&XFS_ISIZE (ip) < eofb->eof_min_file_size)
fs/xfs/xfs_icache.c xfs_inode_free_eofblocks( · line 1247
Quoted 10 lines
fs/xfs/xfs_icache.c +9
-0return 0 ; + +/* +* A scan owner implies we already hold the iolock. Skip it in +* xfs_free_eofblocks() to avoid deadlock. This also eliminates +* the possibility of EAGAIN being returned. +*/ +if (eofb->eof_scan_owner !=NULLFSINO && + eofb->eof_scan_owner == ip->i_ino) + need_iolock = false;
No need to check against NULLFSINO there. ip->i_ino can never be NULLFSINO, so just checking eofb->eof_scan_owner == ip->i_ino is sufficient.
What might be an idea is adding a
ASSERT(eofb->eof_scan_owner != 0);
to the start of the function to catch anyone who does not initialise it appropriately. The inode number can never be zero (that would translate to block 0 - the primary superblock) and so this woul dbe sufficient to ensure callers are doing the right thing....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
In reply to Brian FosterQuoted 4 lines · Brian Foster
The scan owner field represents an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eofblocks. This is an internal only field.
xfs_free_eofblocks already does a trylock, and without that calling it from one iolock holding process to another would be a deadlock waiting to happen.
I have to say I'm still not very easy with iolock nesting, even if it's a trylock.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Brian Foster
The scan owner field represents an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eofblocks. This is an internal only field.
Quoted 7 lines
xfs_free_eofblocksalready does a trylock, and without that calling it from one iolock holding process to another would be a deadlock waiting to happen.I have to say I'm still not very easy with iolock nesting, even if it's a trylock.
Right... maybe I'm not parsing your point. The purpose here is to avoid the trylock entirely. E.g., Indicate that we have already acquired the lock and can proceed with xfs_free_eofblocks(), rather than fail a trylock and skip (which appears to be a potential infinite loop scenario here due to how the AG walking code handles EAGAIN).
Brian
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
Quoted 15 lines · Brian Foster
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Brian Foster
The scan owner field represents an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eofblocks. This is an internal only field.
Quoted 7 lines
xfs_free_eofblocksalready does a trylock, and without that calling it from one iolock holding process to another would be a deadlock waiting to happen.I have to say I'm still not very easy with iolock nesting, even if it's a trylock.
Quoted 5 lines
Right... maybe I'm not parsing your point. The purpose here is to avoid the trylock entirely. E.g., Indicate that we have already acquired the lock and can proceed with
xfs_free_eofblocks(), rather than fail a trylock and skip (which appears to be a potential infinite loop scenario here due to how the AG walking code handles EAGAIN).
I think Christoph's concern here is that we are calling a function that can take the iolock while we already hold the iolock. i.e. the reason we have to add the anti-deadlock code in the first place. To address that, can we restructure xfs_file_buffered_aio_write() such that the ENOSPC/EDQUOT flush is done outside the iolock?
Quoted 1 line
From a quick check, I don't think there is any problem with dropping
the iolock, doing the flushes and then going all the way back to the start of the function again, but closer examination and testing is warranted...
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
Quoted 9 lines · Dave Chinner
Quoted 5 lines
Right... maybe I'm not parsing your point. The purpose here is to avoid the trylock entirely. E.g., Indicate that we have already acquired the lock and can proceed with
xfs_free_eofblocks(), rather than fail a trylock and skip (which appears to be a potential infinite loop scenario here due to how the AG walking code handles EAGAIN).I think Christoph's concern here is that we are calling a function that can take the iolock while we already hold the iolock. i.e. the reason we have to add the anti-deadlock code in the first place.
Indeed.
Quoted 8 lines
To
address that, can we restructurexfs_file_buffered_aio_write()such that the ENOSPC/EDQUOT flush is done outside the iolock?Quoted 1 line
From a quick check, I don't think there is any problem with dropping
the iolock, doing the flushes and then going all the way back to the start of the function again, but closer examination and testing is warranted...
I think we'd need some form of early space reservation, otherwise we'd get non-atomic writes. Time to get those batches write patches out again..
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 1/3] xfs: add scan owner field to xfs_eofblocks
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Dave Chinner
Quoted 5 lines
Right... maybe I'm not parsing your point. The purpose here is to avoid the trylock entirely. E.g., Indicate that we have already acquired the lock and can proceed with
xfs_free_eofblocks(), rather than fail a trylock and skip (which appears to be a potential infinite loop scenario here due to how the AG walking code handles EAGAIN).
Quoted 6 lines
Quoted 3 lines
I think Christoph's concern here is that we are calling a function that can take the iolock while we already hold the iolock. i.e. the reason we have to add the anti-deadlock code in the first place.
Indeed.
Ah, I didn't parse correctly then. Thanks...
Quoted 9 lines
Quoted 8 lines
To
address that, can we restructurexfs_file_buffered_aio_write()such that the ENOSPC/EDQUOT flush is done outside the iolock?Quoted 1 line
From a quick check, I don't think there is any problem with dropping
the iolock, doing the flushes and then going all the way back to the start of the function again, but closer examination and testing is warranted...
I considered this briefly early on, but wasn't sure about whether we should run through the write_checks() bits more than once (e.g., potentially do the eof zeroing, etc., multiple times..?).
Quoted 4 lines
I think we'd need some form of early space reservation, otherwise we'd get non-atomic writes. Time to get those batches write patches out again..
So the concern is that multiple writers to an overlapped range could become interleaved? From passing through the code, we hit generic_perform_write(), which iters over the iov in a write_begin/copy_write_end loop. If we hit ENOSPC somewhere in the middle, we'd return what we've written so far. I don't believe the buffered_aio_write() path would see the error unless it was the first attempt at a delayed allocation. IOW, mid-write failure will be a short write vs. an ENOSPC error.
It seems like it _might_ be safe to drop and reacquire iolock given these semantics (notwithstanding the write_checks() bits), but I could certainly be missing something...
Brian
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/3 xfs: run an eofblocks scan on ENOSPC/EDQUOT
In reply to Brian FosterSpeculative preallocation and and the associated throttling metrics assume we're working with large files on large filesystems. Users have reported inefficiencies in these mechanisms when we happen to be dealing with large files on smaller filesystems. This can occur because while prealloc throttling is aggressive under low free space conditions, it is not active until we reach 5% free space or less.
For example, a 40GB filesystem has enough space for several files large enough to have multi-GB preallocations at any given time. If those files are slow growing, they might reserve preallocation for long periods of time as well as avoid the background scanner due to frequent modification. If a new file is written under these conditions, said file has no access to this already reserved space and premature ENOSPC is imminent.
To handle this scenario, modify the buffered write ENOSPC handling and retry sequence to invoke an eofblocks scan. In the smaller filesystem scenario, the eofblocks scan resets the usage of preallocation such that when the 5% free space threshold is met, throttling effectively takes over to provide fair and efficient preallocation until legitimate ENOSPC.
The eofblocks scan is selective based on the nature of the failure. For example, an EDQUOT failure in a particular quota will use a filtered scan for that quota. Because we don't know which quota might have caused an allocation failure at any given time, we run a scan against each applicable quota determined to be under low free space conditions.
--- fs/xfs/xfs_dquot.h | 15 +++++++++++++++ fs/xfs/xfs_file.c | 23 +++++++++++++++++++---- fs/xfs/xfs_icache.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_icache.h | 1 + 4 files changed, 82 insertions(+), 4 deletions(-)
fs/xfs/xfs_dquot.h +15 -0
@@ -139,6 +139,21 @@ static inline xfs_dquot_t *xfs_inode_dquot(struct xfs_inode *ip, int type) } } +/* +* Check whether a dquot is under low free space conditions. We assume the quota +* is enabled and enforced. +*/ +static inline bool xfs_dquot_lowsp (struct xfs_dquot *dqp) +{ +int64_t freesp; + + freesp =be64_to_cpu (dqp->q_core.d_blk_hardlimit) - dqp->q_res_bcount; +if (freesp < dqp->q_low_space[XFS_QLOWSP_1_PCNT ]) +return true; + +return false; +} +#define XFS_DQ_IS_LOCKED (dqp) (mutex_is_locked(&((dqp)->q_qlock)))#define XFS_DQ_IS_DIRTY (dqp) ((dqp)->dq_flags & XFS_DQ_DIRTY)#define XFS_QM_ISUDQ (dqp) ((dqp)->dq_flags & XFS_DQ_USER)
fs/xfs/xfs_file.c +19 -4
@@ -38,6 +38,7 @@#include "xfs_trace.h" #include "xfs_log.h" #include "xfs_dinode.h" +#include "xfs_icache.h" #include <linux/aio.h> #include <linux/dcache.h>
@@ -741,14 +742,28 @@ write_retry: ret =generic_perform_write (file, &from, pos);if (likely (ret >=0 )) iocb->ki_pos = pos + ret; +/* -* If we just got an ENOSPC, try to write back all dirty inodes to -* convert delalloc space to free up some of the excess reserved -* metadata space. +* If we hit a space limit, try to free up some lingering preallocated +* space before returning an error. In the case of ENOSPC, first try to +* write back all dirty inodes to free up some of the excess reserved +* metadata space. This reduces the chances that the eofblocks scan +* waits on dirty mappings. Since xfs_flush_inodes() is serialized, this +* also behaves as a filter to prevent too many eofblocks scans from +* running at the same time. */ -if (ret == -ENOSPC && !enospc) { +if (ret == -EDQUOT && !enospc) { + enospc =xfs_inode_free_quota_eofblocks (ip); +if (enospc) + goto write_retry; + }else if (ret == -ENOSPC && !enospc) { +struct xfs_eofblocks eofb = {0 }; + enospc =1 ;xfs_flush_inodes (ip->i_mount); + eofb.eof_scan_owner = ip->i_ino;/* for locking */ + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; +xfs_icache_free_eofblocks (ip->i_mount, &eofb); goto write_retry; }
fs/xfs/xfs_icache.c +47 -0
59 lines not on this page · raw message
fs/xfs/xfs_icache.h +1 -0
@@ -58,6 +58,7 @@ void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);void xfs_inode_set_eofblocks_tag (struct xfs_inode *ip);void xfs_inode_clear_eofblocks_tag (struct xfs_inode *ip);int xfs_icache_free_eofblocks (struct xfs_mount *,struct xfs_eofblocks *); +int xfs_inode_free_quota_eofblocks (struct xfs_inode *ip);void xfs_eofblocks_worker (struct work_struct *); int xfs_inode_ag_iterator(struct xfs_mount *mp,
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT
Quoted 155 lines · Brian Foster
Speculative preallocation and and the associated throttling metrics assume we're working with large files on large filesystems. Users have reported inefficiencies in these mechanisms when we happen to be dealing with large files on smaller filesystems. This can occur because while prealloc throttling is aggressive under low free space conditions, it is not active until we reach 5% free space or less.
For example, a 40GB filesystem has enough space for several files large enough to have multi-GB preallocations at any given time. If those files are slow growing, they might reserve preallocation for long periods of time as well as avoid the background scanner due to frequent modification. If a new file is written under these conditions, said file has no access to this already reserved space and premature ENOSPC is imminent.
To handle this scenario, modify the buffered write ENOSPC handling and retry sequence to invoke an eofblocks scan. In the smaller filesystem scenario, the eofblocks scan resets the usage of preallocation such that when the 5% free space threshold is met, throttling effectively takes over to provide fair and efficient preallocation until legitimate ENOSPC.
The eofblocks scan is selective based on the nature of the failure. For example, an EDQUOT failure in a particular quota will use a filtered scan for that quota. Because we don't know which quota might have caused an allocation failure at any given time, we run a scan against each applicable quota determined to be under low free space conditions.
Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_dquot.h | 15 +++++++++++++++ fs/xfs/xfs_file.c | 23 +++++++++++++++++++---- fs/xfs/xfs_icache.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_icache.h | 1 + 4 files changed, 82 insertions(+), 4 deletions(-)fs/xfs/xfs_dquot.h +15
-0@@ -139,6 +139,21 @@ static inline xfs_dquot_t *xfs_inode_dquot(struct xfs_inode *ip, int type) } } +/* +* Check whether a dquot is under low free space conditions. We assume the quota +* is enabled and enforced. +*/ +static inline bool xfs_dquot_lowsp (struct xfs_dquot *dqp) +{ +int64_t freesp; + + freesp =be64_to_cpu (dqp->q_core.d_blk_hardlimit) - dqp->q_res_bcount; +if (freesp < dqp->q_low_space[XFS_QLOWSP_1_PCNT ]) +return true; + +return false; +} +#define XFS_DQ_IS_LOCKED (dqp) (mutex_is_locked(&((dqp)->q_qlock)))#define XFS_DQ_IS_DIRTY (dqp) ((dqp)->dq_flags & XFS_DQ_DIRTY)#define XFS_QM_ISUDQ (dqp) ((dqp)->dq_flags & XFS_DQ_USER)fs/xfs/xfs_file.c +19
-4@@ -38,6 +38,7 @@#include "xfs_trace.h" #include "xfs_log.h" #include "xfs_dinode.h" +#include "xfs_icache.h" #include <linux/aio.h> #include <linux/dcache.h> @@ -741,14 +742,28 @@ write_retry: ret =generic_perform_write (file, &from, pos);if (likely (ret >=0 )) iocb->ki_pos = pos + ret; +/* -* If we just got an ENOSPC, try to write back all dirty inodes to -* convert delalloc space to free up some of the excess reserved -* metadata space. +* If we hit a space limit, try to free up some lingering preallocated +* space before returning an error. In the case of ENOSPC, first try to +* write back all dirty inodes to free up some of the excess reserved +* metadata space. This reduces the chances that the eofblocks scan +* waits on dirty mappings. Since xfs_flush_inodes() is serialized, this +* also behaves as a filter to prevent too many eofblocks scans from +* running at the same time. */ -if (ret == -ENOSPC && !enospc) { +if (ret == -EDQUOT && !enospc) { + enospc =xfs_inode_free_quota_eofblocks (ip); +if (enospc) + goto write_retry; + }else if (ret == -ENOSPC && !enospc) { +struct xfs_eofblocks eofb = {0 }; + enospc =1 ;xfs_flush_inodes (ip->i_mount); + eofb.eof_scan_owner = ip->i_ino;/* for locking */ + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; +xfs_icache_free_eofblocks (ip->i_mount, &eofb); goto write_retry; }fs/xfs/xfs_icache.c +33
-0@@ -33,6 +33,9 @@#include "xfs_trace.h" #include "xfs_icache.h" #include "xfs_bmap_util.h" +#include "xfs_quota.h" +#include "xfs_dquot_item.h" +#include "xfs_dquot.h" #include <linux/kthread.h> #include <linux/freezer.h> @@ -1270,6 +1273,50 @@ xfs_icache_free_eofblocks( eofb, XFS_ICI_EOFBLOCKS_TAG); } +/* +* 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 ; + } + } +
fs/xfs/xfs_icache.c xfs_icache_free_eofblocks( · line 1315
Quoted 10 lines
fs/xfs/xfs_icache.c +10
-0+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.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT
Quoted 185 lines · Dave Chinner
Quoted 155 lines · Brian Foster
Speculative preallocation and and the associated throttling metrics assume we're working with large files on large filesystems. Users have reported inefficiencies in these mechanisms when we happen to be dealing with large files on smaller filesystems. This can occur because while prealloc throttling is aggressive under low free space conditions, it is not active until we reach 5% free space or less.
For example, a 40GB filesystem has enough space for several files large enough to have multi-GB preallocations at any given time. If those files are slow growing, they might reserve preallocation for long periods of time as well as avoid the background scanner due to frequent modification. If a new file is written under these conditions, said file has no access to this already reserved space and premature ENOSPC is imminent.
To handle this scenario, modify the buffered write ENOSPC handling and retry sequence to invoke an eofblocks scan. In the smaller filesystem scenario, the eofblocks scan resets the usage of preallocation such that when the 5% free space threshold is met, throttling effectively takes over to provide fair and efficient preallocation until legitimate ENOSPC.
The eofblocks scan is selective based on the nature of the failure. For example, an EDQUOT failure in a particular quota will use a filtered scan for that quota. Because we don't know which quota might have caused an allocation failure at any given time, we run a scan against each applicable quota determined to be under low free space conditions.
Signed-off-by Brian Foster <[email protected]>--- fs/xfs/xfs_dquot.h | 15 +++++++++++++++ fs/xfs/xfs_file.c | 23 +++++++++++++++++++---- fs/xfs/xfs_icache.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ fs/xfs/xfs_icache.h | 1 + 4 files changed, 82 insertions(+), 4 deletions(-)fs/xfs/xfs_dquot.h +15
-0@@ -139,6 +139,21 @@ static inline xfs_dquot_t *xfs_inode_dquot(struct xfs_inode *ip, int type) } } +/* +* Check whether a dquot is under low free space conditions. We assume the quota +* is enabled and enforced. +*/ +static inline bool xfs_dquot_lowsp (struct xfs_dquot *dqp) +{ +int64_t freesp; + + freesp =be64_to_cpu (dqp->q_core.d_blk_hardlimit) - dqp->q_res_bcount; +if (freesp < dqp->q_low_space[XFS_QLOWSP_1_PCNT ]) +return true; + +return false; +} +#define XFS_DQ_IS_LOCKED (dqp) (mutex_is_locked(&((dqp)->q_qlock)))#define XFS_DQ_IS_DIRTY (dqp) ((dqp)->dq_flags & XFS_DQ_DIRTY)#define XFS_QM_ISUDQ (dqp) ((dqp)->dq_flags & XFS_DQ_USER)fs/xfs/xfs_file.c +19
-4@@ -38,6 +38,7 @@#include "xfs_trace.h" #include "xfs_log.h" #include "xfs_dinode.h" +#include "xfs_icache.h" #include <linux/aio.h> #include <linux/dcache.h> @@ -741,14 +742,28 @@ write_retry: ret =generic_perform_write (file, &from, pos);if (likely (ret >=0 )) iocb->ki_pos = pos + ret; +/* -* If we just got an ENOSPC, try to write back all dirty inodes to -* convert delalloc space to free up some of the excess reserved -* metadata space. +* If we hit a space limit, try to free up some lingering preallocated +* space before returning an error. In the case of ENOSPC, first try to +* write back all dirty inodes to free up some of the excess reserved +* metadata space. This reduces the chances that the eofblocks scan +* waits on dirty mappings. Since xfs_flush_inodes() is serialized, this +* also behaves as a filter to prevent too many eofblocks scans from +* running at the same time. */ -if (ret == -ENOSPC && !enospc) { +if (ret == -EDQUOT && !enospc) { + enospc =xfs_inode_free_quota_eofblocks (ip); +if (enospc) + goto write_retry; + }else if (ret == -ENOSPC && !enospc) { +struct xfs_eofblocks eofb = {0 }; + enospc =1 ;xfs_flush_inodes (ip->i_mount); + eofb.eof_scan_owner = ip->i_ino;/* for locking */ + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; +xfs_icache_free_eofblocks (ip->i_mount, &eofb); goto write_retry; }fs/xfs/xfs_icache.c +33
-0@@ -33,6 +33,9 @@#include "xfs_trace.h" #include "xfs_icache.h" #include "xfs_bmap_util.h" +#include "xfs_quota.h" +#include "xfs_dquot_item.h" +#include "xfs_dquot.h" #include <linux/kthread.h> #include <linux/freezer.h> @@ -1270,6 +1273,50 @@ xfs_icache_free_eofblocks( eofb, XFS_ICI_EOFBLOCKS_TAG); } +/* +* 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 ; + } + } +fs/xfs/xfs_icache.c xfs_icache_free_eofblocks( · line 1315
Quoted 10 lines
fs/xfs/xfs_icache.c +10
-0+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);
Quoted 5 lines
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?
Brian
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT
Quoted 88 lines · Brian Foster
Quoted 60 lines · Dave Chinner
Quoted 30 lines · Brian Foster
fs/xfs/xfs_icache.c +30
-0+/* +* 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 ; + } + } +fs/xfs/xfs_icache.c xfs_icache_free_eofblocks( · line 1315
Quoted 10 lines
fs/xfs/xfs_icache.c +10
-0+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);Quoted 5 lines
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.
Quoted 1 line
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 2/3] xfs: run an eofblocks scan on ENOSPC/EDQUOT
Quoted 91 lines · Dave Chinner
Quoted 88 lines · Brian Foster
Quoted 60 lines · Dave Chinner
Quoted 30 lines · Brian Foster
fs/xfs/xfs_icache.c +30
-0+/* +* 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 ; + } + } +fs/xfs/xfs_icache.c xfs_icache_free_eofblocks( · line 1315
Quoted 10 lines
fs/xfs/xfs_icache.c +10
-0+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);Quoted 5 lines
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.
Quoted 1 line
I need to think about this some more. Thoughts on any of that?
Quoted 9 lines
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
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 3/3 xfs: squash prealloc while over quota free space as well
In reply to Brian FosterCommit 4d559a3b introduced heavy prealloc. squashing to catch the case of requesting too large a prealloc on smaller filesystems, leading to repeated flush and retry cycles that occur on ENOSPC. Now that we issue eofblocks scans on EDQUOT/ENOSPC, squash the prealloc against the minimum available free space across all applicable quotas as well to avoid a similar problem of repeated eofblocks scans.
--- fs/xfs/xfs_iomap.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
fs/xfs/xfs_iomap.c +14 -6
@@ -397,7 +397,8 @@ xfs_quota_calc_throttle(struct xfs_inode *ip, int type, xfs_fsblock_t *qblocks, - int *qshift) + int *qshift, + int64_t *qfreesp) { int64_t freesp;int shift =0 ;
@@ -406,6 +407,7 @@ xfs_quota_calc_throttle(/* over hi wmark, squash the prealloc completely */ if (dq->q_res_bcount >= dq->q_prealloc_hi_wmark) { *qblocks =0 ; + *qfreesp =0 ;return ; }
@@ -418,6 +420,9 @@ xfs_quota_calc_throttle( shift +=2 ; } +if (freesp < *qfreesp) + *qfreesp = freesp; +/* only overwrite the throttle values if we are more aggressive */ if ((freesp >> shift) < (*qblocks >> *qshift)) { *qblocks = freesp;
@@ -476,15 +481,18 @@ xfs_iomap_prealloc_size( }/* -* Check each quota to cap the prealloc size and provide a shift -* value to throttle with. +* Check each quota to cap the prealloc size, provide a shift value to +* throttle with and adjust amount of available space. */ if (xfs_quota_need_throttle (ip,XFS_DQ_USER , alloc_blocks)) -xfs_quota_calc_throttle (ip,XFS_DQ_USER , &qblocks, &qshift); +xfs_quota_calc_throttle (ip,XFS_DQ_USER , &qblocks, &qshift, + &freesp);if (xfs_quota_need_throttle (ip,XFS_DQ_GROUP , alloc_blocks)) -xfs_quota_calc_throttle (ip,XFS_DQ_GROUP , &qblocks, &qshift); +xfs_quota_calc_throttle (ip,XFS_DQ_GROUP , &qblocks, &qshift, + &freesp);if (xfs_quota_need_throttle (ip,XFS_DQ_PROJ , alloc_blocks)) -xfs_quota_calc_throttle (ip,XFS_DQ_PROJ , &qblocks, &qshift); +xfs_quota_calc_throttle (ip,XFS_DQ_PROJ , &qblocks, &qshift, + &freesp);/* * The final prealloc size is set to the minimum of free space available
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH v2 3/3] xfs: squash prealloc while over quota free space as well
Quoted 8 lines · Brian Foster
Commit
4d559a3bintroduced heavy prealloc. squashing to catch the case of requesting too large a prealloc on smaller filesystems, leading to repeated flush and retry cycles that occur on ENOSPC. Now that we issue eofblocks scans on EDQUOT/ENOSPC, squash the prealloc against the minimum available free space across all applicable quotas as well to avoid a similar problem of repeated eofblocks scans.Signed-off-by Brian Foster <[email protected]>
Good idea - thrashing is something worth avoiding....
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs