* [PATCH] btrfs: Always check nocow for quota enabled case to make sure we won't reserve unnecessary data space
@ 2018-08-15 6:13 Qu Wenruo
2018-08-21 4:21 ` Misono Tomohiro
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2018-08-15 6:13 UTC (permalink / raw)
To: linux-btrfs
Commit c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
makes nocow check less frequent to improve performance.
However for quota enabled case, such optimization could lead to extra
unnecessary data reservation, which results failure for test case like
btrfs/153 in fstests.
Fix it by reverting to old behavior for quota enabled case.
Fixes: c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
fs/btrfs/file.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 51e77d72068a..f2ce1d707d4c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1587,6 +1587,7 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
int ret = 0;
bool only_release_metadata = false;
bool force_page_uptodate = false;
+ bool quota_enabled = test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE),
PAGE_SIZE / (sizeof(struct page *)));
@@ -1627,9 +1628,15 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
fs_info->sectorsize);
extent_changeset_release(data_reserved);
- ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
+ /*
+ * If we have quota enabled, we must do the heavy lift nocow
+ * check here to avoid reserving data space, or we can hit
+ * limitation for NOCOW files.
+ */
+ if (!quota_enabled)
+ ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
write_bytes);
- if (ret < 0) {
+ if (ret < 0 || quota_enabled) {
if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
BTRFS_INODE_PREALLOC)) &&
check_can_nocow(BTRFS_I(inode), pos,
--
2.18.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs: Always check nocow for quota enabled case to make sure we won't reserve unnecessary data space
2018-08-15 6:13 [PATCH] btrfs: Always check nocow for quota enabled case to make sure we won't reserve unnecessary data space Qu Wenruo
@ 2018-08-21 4:21 ` Misono Tomohiro
2018-08-21 4:57 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: Misono Tomohiro @ 2018-08-21 4:21 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 2018/08/15 15:13, Qu Wenruo wrote:
> Commit c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
> makes nocow check less frequent to improve performance.
>
> However for quota enabled case, such optimization could lead to extra
> unnecessary data reservation, which results failure for test case like
> btrfs/153 in fstests.
>
> Fix it by reverting to old behavior for quota enabled case.
>
> Fixes: c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/file.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 51e77d72068a..f2ce1d707d4c 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1587,6 +1587,7 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
> int ret = 0;
> bool only_release_metadata = false;
> bool force_page_uptodate = false;
> + bool quota_enabled = test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>
> nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE),
> PAGE_SIZE / (sizeof(struct page *)));
> @@ -1627,9 +1628,15 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
> fs_info->sectorsize);
>
> extent_changeset_release(data_reserved);
> - ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
> + /*
> + * If we have quota enabled, we must do the heavy lift nocow
> + * check here to avoid reserving data space, or we can hit
> + * limitation for NOCOW files.
> + */
> + if (!quota_enabled)
> + ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
> write_bytes);
> - if (ret < 0) {
> + if (ret < 0 || quota_enabled) {
> if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> BTRFS_INODE_PREALLOC)) &&> check_can_nocow(BTRFS_I(inode), pos,
>
This fixes btrfs/153 but in turn makes other qgroup tests fail (022,091 etc.)
If quota is enabled and file is not marked as nocow, above if is false and write will stop.
so I think we still need to call btrfs_check_dta_free_space() when quota is enabled
and file is not nocow, right?
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs: Always check nocow for quota enabled case to make sure we won't reserve unnecessary data space
2018-08-21 4:21 ` Misono Tomohiro
@ 2018-08-21 4:57 ` Qu Wenruo
0 siblings, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2018-08-21 4:57 UTC (permalink / raw)
To: Misono Tomohiro, Qu Wenruo, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 2617 bytes --]
On 2018/8/21 下午12:21, Misono Tomohiro wrote:
> On 2018/08/15 15:13, Qu Wenruo wrote:
>> Commit c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
>> makes nocow check less frequent to improve performance.
>>
>> However for quota enabled case, such optimization could lead to extra
>> unnecessary data reservation, which results failure for test case like
>> btrfs/153 in fstests.
>>
>> Fix it by reverting to old behavior for quota enabled case.
>>
>> Fixes: c6887cd11149 ("Btrfs: don't do nocow check unless we have to")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> fs/btrfs/file.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index 51e77d72068a..f2ce1d707d4c 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -1587,6 +1587,7 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>> int ret = 0;
>> bool only_release_metadata = false;
>> bool force_page_uptodate = false;
>> + bool quota_enabled = test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>
>> nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE),
>> PAGE_SIZE / (sizeof(struct page *)));
>> @@ -1627,9 +1628,15 @@ static noinline ssize_t __btrfs_buffered_write(struct file *file,
>> fs_info->sectorsize);
>>
>> extent_changeset_release(data_reserved);
>> - ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
>> + /*
>> + * If we have quota enabled, we must do the heavy lift nocow
>> + * check here to avoid reserving data space, or we can hit
>> + * limitation for NOCOW files.
>> + */
>> + if (!quota_enabled)
>> + ret = btrfs_check_data_free_space(inode, &data_reserved, pos,
>> write_bytes);
>> - if (ret < 0) {
>> + if (ret < 0 || quota_enabled) {
>> if ((BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
>> BTRFS_INODE_PREALLOC)) &&> check_can_nocow(BTRFS_I(inode), pos,
>>
>
> This fixes btrfs/153 but in turn makes other qgroup tests fail (022,091 etc.)
Thanks for the test.
I also found the problem too.
>
> If quota is enabled and file is not marked as nocow, above if is false and write will stop.
> so I think we still need to call btrfs_check_dta_free_space() when quota is enabled
> and file is not nocow, right?
Yes, we should btrfs_check_data_free_space() check.
I only considered quota+nocow and noquota case.
For quota+cow case it should goes the normal way.
I'll check if there is any better solution to organize the code.
Thanks,
Qu
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-21 8:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-15 6:13 [PATCH] btrfs: Always check nocow for quota enabled case to make sure we won't reserve unnecessary data space Qu Wenruo
2018-08-21 4:21 ` Misono Tomohiro
2018-08-21 4:57 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).