xfs: flush CoW fork reservations before processing quota get request
PATCH 1/1 xfs: flush CoW fork reservations before processing quota get request
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork. xfs_bmap_extsize_align() calculates XFS_DEFAULT_COWEXTSZ_HINT (32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e. i_size), 1M + 16 blocks] is freed by __fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.
The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
fs/xfs/xfs_quotaops.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
fs/xfs/xfs_quotaops.c +13 -0
@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }
@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);
Signature · 1 line
2.9.5
Quoted 11 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.
This still refers to the COW fork, right?
Quoted 5 lines
The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
And so this refers to cowblocks within EOF..? If so, that means those blocks are consumed if that particular range of the file is written as well. The above sort of reads like they'd stick around without any real purpose, which is either a bit confusing or suggests I'm missing something.
This also all sounds like expected behavior to this point..
Quoted 3 lines
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Could you elaborate more on the fundamental problem wrt to quota? Are the cow blocks not accounted properly or something? What exactly makes this a problem with 64k page sizes and not the more common 4k page/block size?
Quoted 23 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +2
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };
fs/xfs/xfs_quotaops.c xfs_fs_get_dqblk( · line 235
Quoted 10 lines
fs/xfs/xfs_quotaops.c +5
-0if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; +
So this is a full scan of the in-core icache per call. I'm not terribly familiar with the quota infrastructure code, but just from the context it looks like this is per quota id. The eofblocks infrastructure supports id filtering, which makes me wonder (at minimum) why we wouldn't limit the scan to the id associated with the quota?
Brian
Quoted 25 lines
fs/xfs/xfs_quotaops.c +6
-0id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);Signature · 1 line
2.9.5
Quoted 13 lines · IST Brian Foster
Quoted 11 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.
Quoted 1 line
This still refers to the COW fork, right?
Yes, xfs_itruncate_extents_flags() invokes xfs_reflink_cancel_cow_blocks() when "data fork" is being truncated.
Quoted 7 lines
Quoted 6 lines
The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
Quoted 5 lines
And so this refers to cowblocks within EOF..? If so, that means those blocks are consumed if that particular range of the file is written as well. The above sort of reads like they'd stick around without any real purpose, which is either a bit confusing or suggests I'm missing something.
Yes, the above mentioned range (within inode->i_isize) does not have any data written to. The space was speculatively reserved.
Quoted 11 lines
This also all sounds like expected behavior to this point..
Quoted 3 lines
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Could you elaborate more on the fundamental problem wrt to quota? Are the cow blocks not accounted properly or something? What exactly makes this a problem with 64k page sizes and not the more common 4k page/block size?
The speculative allocation of CoW blocks are in units of blocks. The default CoW extent size hint is set to XFS_DEFAULT_COWEXTSZ_HINT (i.e. 32 blocks). For 4k block size this equals 131072 bytes while for 64k block size it is 2097152 bytes.
generic/305 initially creates 1MiB file. It then creates another file which shares its data blocks with the original file. The test then writes 512K worth of data at file range [0, 512k-1]. Now here is where we have a difference b/w 4k v/s 64k block sized filesystems.
Writing 512k data causes max(data written, 32 blocks) of space to be reserved in the CoW fork i.e 512k bytes for 4k block FS and 2097152 bytes for 64k block FS. On 4k block FS, the reservation in CoW fork gets cleared when 512k bytes of data are written to disk. However for 64k block FS, 2097152 - 512k = 1572864 bytes remain in CoW fork until either the CoW space trimming worker gets triggered or until the filesystem is umounted.
Quoted 29 lines
Quoted 29 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +2
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ;
Quoted 11 lines
fs/xfs/xfs_quotaops.c xfs_fs_get_dqblk( · line 235
Quoted 5 lines
fs/xfs/xfs_quotaops.c +5
-0+ eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; +So this is a full scan of the in-core icache per call. I'm not terribly familiar with the quota infrastructure code, but just from the context it looks like this is per quota id. The eofblocks infrastructure supports id filtering, which makes me wonder (at minimum) why we wouldn't limit the scan to the id associated with the quota?
I now think replacing the call to "$XFS_SPACEMAN_PROG -c 'prealloc -s' call" in _check_quota_usage() with umount/mount cycle is the right thing to do.
Quoting my response to Darrick's mail,
;; Hmm. W.r.t Preallocated EOF blocks, it is easy to identify the blocks to be ;; removed by the ioctl i.e. blocks which are present beyond inode->i_size.
;; You are right about the inability to do so for CoW blocks since some of the ;; unused CoW blocks fall within inode->i_size. Hence I agree with your approach ;; of replacing "$XFS_SPACEMAN_PROG -c 'prealloc -s' call' in _check_quota_usage ;; with umount/mount.
Quoted 27 lines
Brian
Quoted 22 lines
fs/xfs/xfs_quotaops.c +6
-0id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);
Signature · 1 line
chandan
Quoted 26 lines · Chandan Rajendra
Quoted 13 lines · IST Brian Foster
Quoted 11 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.Quoted 1 line
This still refers to the COW fork, right?
Yes,
xfs_itruncate_extents_flags()invokesxfs_reflink_cancel_cow_blocks()when "data fork" is being truncated.Quoted 7 lines
Quoted 6 lines
The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
Quoted 9 lines
Quoted 5 lines
And so this refers to cowblocks within EOF..? If so, that means those blocks are consumed if that particular range of the file is written as well. The above sort of reads like they'd stick around without any real purpose, which is either a bit confusing or suggests I'm missing something.
Yes, the above mentioned range (within
inode->i_isize) does not have any data written to. The space was speculatively reserved.
Sure, that might be true of the test case, but the purpose of allocation hint is essentially to speculate on future writes. Without it, a set of small and scattered writes over a range of shared blocks in a file results in about equally as many small allocations and can fragment the file.
Quoted 17 lines
Quoted 11 lines
This also all sounds like expected behavior to this point..
Quoted 3 lines
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Could you elaborate more on the fundamental problem wrt to quota? Are the cow blocks not accounted properly or something? What exactly makes this a problem with 64k page sizes and not the more common 4k page/block size?
The speculative allocation of CoW blocks are in units of blocks. The default CoW extent size hint is set to
XFS_DEFAULT_COWEXTSZ_HINT(i.e. 32 blocks). For 4k block size this equals 131072 bytes while for 64k block size it is 2097152 bytes.
Quoted 5 lines
generic/305 initially creates 1MiB file. It then creates another file which shares its data blocks with the original file. The test then writes 512K worth of data at file range [0, 512k-1]. Now here is where we have a difference b/w 4k v/s 64k block sized filesystems.
Ok..
Quoted 7 lines
Writing 512k data causes
max(data written, 32 blocks)of space to be reserved in the CoW fork i.e 512k bytes for 4k block FS and 2097152 bytes for 64k block FS. On 4k block FS, the reservation in CoW fork gets cleared when 512k bytes of data are written to disk. However for 64k block FS, 2097152 - 512k = 1572864 bytes remain in CoW fork until either the CoW space trimming worker gets triggered or until the filesystem is umounted.
Yep, but this strikes me as an implementation detail of the test. IOW, if the test issued a smaller write that didn't fully consume the 32-block allocation hint with 4k blocks, we'd be in the same state.
So this patch implies that there's some kind of problem with quota stats/reporting with active COW fork reservations but doesn't actually explain what it is.
Quoted 42 lines
Quoted 30 lines
Quoted 30 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +2
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ;Quoted 11 lines
fs/xfs/xfs_quotaops.c xfs_fs_get_dqblk( · line 235
Quoted 5 lines
fs/xfs/xfs_quotaops.c +5
-0+ eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; +So this is a full scan of the in-core icache per call. I'm not terribly familiar with the quota infrastructure code, but just from the context it looks like this is per quota id. The eofblocks infrastructure supports id filtering, which makes me wonder (at minimum) why we wouldn't limit the scan to the id associated with the quota?
Quoted 3 lines
I now think replacing the call to "$XFS_SPACEMAN_PROG -c 'prealloc -s' call" in
_check_quota_usage()with umount/mount cycle is the right thing to do.
Ok. Sounds like it's a test issue one way or another then...
Brian
Quoted 43 lines
Quoting my response to Darrick's mail,
;; Hmm. W.r.t Preallocated EOF blocks, it is easy to identify the blocks to be ;; removed by the ioctl i.e. blocks which are present beyond
inode->i_size.;; You are right about the inability to do so for CoW blocks since some of the ;; unused CoW blocks fall within
inode->i_size. Hence I agree with your approach ;; of replacing "$XFS_SPACEMAN_PROG -c 'prealloc -s' call' in_check_quota_usage;; with umount/mount.Quoted 27 lines
Brian
Quoted 22 lines
fs/xfs/xfs_quotaops.c +6
-0id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);Signature · 1 line
chandan
Quoted 19 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Quoted 6 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
Hmmm. I restarted looking into all the weird quota count mismatches in xfstests and noticed (with a generous amount of trace_printks) that most of the discrepancies can be traced to speculative preallocations in the cow fork that don't get cleaned out. So we're on the same page. :)
I thought about enhancing the XFS_IOC_FREE_EOFBLOCKS ioctl with a new mode to clean out CoW stuff too, but then I started thinking about what _check_quota_usage is actually looking for, and realized that (for xfs anyway) it compares an aged quota report (reflective of thousands of individual fs ops) against a freshly quotacheck'd quota report to look for accounting leaks.
Then I tried replacing the $XFS_SPACEMAN_PROG -c 'prealloc -s' call in _check_quota_usage with a umount/mount cycle so that we know we've cleaned out all the reservations and *poof* the discrepancies all went away. The test is still useful since we're comparing the accumulated quota counts against freshly computed counts, but now we know that we've cleaned out any speculative preallocations that xfs might have decided to try (assuming xfs never changes behavior to speculate on a fresh mount).
It's awfully tempting to just leave it that way... but what do you think? I think it's a better solution than forcing /every/ quota report to iterate the in-core inodes looking for cow blocks to dump.
Granted maybe we still want the ioctl to do it for us? Though that could get tricky since written extents in the cow fork represent writes in progress and can't ever be removed except by xfs_inactive.
--D
Quoted 51 lines
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +13
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);Signature · 1 line
2.9.5
Quoted 48 lines · IST Darrick J. Wong
Quoted 19 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Quoted 6 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
Hmmm. I restarted looking into all the weird quota count mismatches in xfstests and noticed (with a generous amount of
trace_printks) that most of the discrepancies can be traced to speculative preallocations in the cow fork that don't get cleaned out. So we're on the same page. :)I thought about enhancing the
XFS_IOC_FREE_EOFBLOCKSioctl with a new mode to clean out CoW stuff too, but then I started thinking about what_check_quota_usageis actually looking for, and realized that (for xfs anyway) it compares an aged quota report (reflective of thousands of individual fs ops) against a freshly quotacheck'd quota report to look for accounting leaks.Then I tried replacing the $XFS_SPACEMAN_PROG -c 'prealloc -s' call in
_check_quota_usagewith a umount/mount cycle so that we know we've cleaned out all the reservations and *poof* the discrepancies all went away. The test is still useful since we're comparing the accumulated quota counts against freshly computed counts, but now we know that we've cleaned out any speculative preallocations that xfs might have decided to try (assuming xfs never changes behavior to speculate on a fresh mount).
Quoted 7 lines
It's awfully tempting to just leave it that way... but what do you think? I think it's a better solution than forcing /every/ quota report to iterate the in-core inodes looking for cow blocks to dump.
Granted maybe we still want the ioctl to do it for us? Though that could get tricky since written extents in the cow fork represent writes in progress and can't ever be removed except by
xfs_inactive.
Hmm. W.r.t Preallocated EOF blocks, it is easy to identify the blocks to be removed by the ioctl i.e. blocks which are present beyond inode->i_size.
You are right about the inability to do so for CoW blocks since some of the unused CoW blocks fall within inode->i_size. Hence I agree with your approach of replacing "$XFS_SPACEMAN_PROG -c 'prealloc -s' call' in _check_quota_usage with umount/mount.
If you are fine with it, I can fix _check_quota_usage() and also the relevant tests.
Quoted 51 lines
Quoted 49 lines
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +13
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);
Signature · 1 line
chandan
Quoted 60 lines · Chandan Rajendra
Quoted 48 lines · IST Darrick J. Wong
Quoted 19 lines · Chandan Rajendra
generic/305 fails on a 64k block sized filesystem due to the following interaction,
1. We are writing 8 blocks (i.e. [0, 512k-1]) of data to a 1 MiB file.
2. XFS reserves 32 blocks of space in the CoW fork.xfs_bmap_extsize_align()calculatesXFS_DEFAULT_COWEXTSZ_HINT(32 blocks) as the number of blocks to be reserved.
3. The reserved space in the range [1M(i.e.i_size), 1M + 16 blocks] is freed by__fput(). This corresponds to freeing "eof blocks" i.e. space reserved beyond EOF of a file.The reserved space to which data was never written i.e. [9th block, 1M(EOF)], remains reserved in the CoW fork until either the CoW block reservation trimming worker gets invoked or the filesystem is unmounted.
This commit fixes the issue by freeing unused CoW block reservations whenever quota numbers are requested by userspace application.
Quoted 6 lines
Signed-off-by Chandan Rajendra <[email protected]>---
PS: With the above patch, the tests xfs/214 & xfs/440 fail because the value passed to xfs_io's cowextsize does not have any effect when CoW fork reservations are flushed before querying for quota usage numbers.
Hmmm. I restarted looking into all the weird quota count mismatches in xfstests and noticed (with a generous amount of
trace_printks) that most of the discrepancies can be traced to speculative preallocations in the cow fork that don't get cleaned out. So we're on the same page. :)I thought about enhancing the
XFS_IOC_FREE_EOFBLOCKSioctl with a new mode to clean out CoW stuff too, but then I started thinking about what_check_quota_usageis actually looking for, and realized that (for xfs anyway) it compares an aged quota report (reflective of thousands of individual fs ops) against a freshly quotacheck'd quota report to look for accounting leaks.Then I tried replacing the $XFS_SPACEMAN_PROG -c 'prealloc -s' call in
_check_quota_usagewith a umount/mount cycle so that we know we've cleaned out all the reservations and *poof* the discrepancies all went away. The test is still useful since we're comparing the accumulated quota counts against freshly computed counts, but now we know that we've cleaned out any speculative preallocations that xfs might have decided to try (assuming xfs never changes behavior to speculate on a fresh mount).Quoted 7 lines
It's awfully tempting to just leave it that way... but what do you think? I think it's a better solution than forcing /every/ quota report to iterate the in-core inodes looking for cow blocks to dump.
Granted maybe we still want the ioctl to do it for us? Though that could get tricky since written extents in the cow fork represent writes in progress and can't ever be removed except by
xfs_inactive.Hmm. W.r.t Preallocated EOF blocks, it is easy to identify the blocks to be removed by the ioctl i.e. blocks which are present beyond
inode->i_size.
Quoted 7 lines
You are right about the inability to do so for CoW blocks since some of the unused CoW blocks fall within
inode->i_size. Hence I agree with your approach of replacing "$XFS_SPACEMAN_PROG -c 'prealloc -s' call' in_check_quota_usagewith umount/mount.If you are fine with it, I can fix
_check_quota_usage()and also the relevant tests.
I've been testing such a patch for a while (along with a bunch of other quota fixes) so I'll just shove that out for review today.
--D
Quoted 56 lines
Quoted 51 lines
Quoted 49 lines
fs/xfs/xfs_quotaops.c| 13 +++++++++++++1 file changed, 13 insertions(+)fs/xfs/xfs_quotaops.c +13
-0@@ -218,14 +218,21 @@ xfs_fs_get_dqblk(struct kqid qid, struct qc_dqblk *qdq) { + int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, qid);return xfs_qm_scall_getquota (mp, id,xfs_quota_type (qid.type), qdq); }@@ -240,12 +247,18 @@ xfs_fs_get_nextdqblk(int ret;struct xfs_mount *mp =XFS_M (sb);xfs_dqid_t id; +struct xfs_eofblocks eofb = {0 };if (!XFS_IS_QUOTA_RUNNING (mp))return -ENOSYS ;if (!XFS_IS_QUOTA_ON (mp))return -ESRCH ; + eofb.eof_flags =XFS_EOF_FLAGS_SYNC ; + ret =xfs_icache_free_cowblocks (mp, &eofb); +if (ret) +return ret; + id =from_kqid (&init_user_ns, *qid); ret =xfs_qm_scall_getquota_next (mp, &id,xfs_quota_type (qid->type), qdq);Signature · 1 line
chandan