[PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[BUG]
The following command can lead to unexpected data COW:

  #!/bin/bash

  dev=/dev/test/test
  mnt=/mnt/btrfs

  mkfs.btrfs -f $dev -b 1G > /dev/null
  mount $dev $mnt -o nospace_cache

  xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
  xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
  umount $dev

The result extent will be

	item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
		generation 6 type 2 (prealloc)
		prealloc data disk byte 13631488 nr 28672
	item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
		generation 6 type 1 (regular)
		extent data disk byte 13660160 nr 12288 <<< COW
	item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
		generation 6 type 2 (prealloc)
		prealloc data disk byte 13631488 nr 28672

Currently we always reserve space even for NOCOW buffered write, thus
under most case it shouldn't cause anything wrong even we fall back to
COW.

However when we're out of data space, we fall back to skip data space if
we can do NOCOW write.

If such behavior happens under that case, we could hit the following
problems:
- data space bytes_may_use underflow
  This will cause kernel warning.

- ENOSPC at delalloc time
  This will lead to transaction abort and fs forced to RO.

[CAUSE]
This is due to the fact that btrfs can only do extent level share check.

Btrfs can only tell if an extent is shared, no matter if only part of the
extent is shared or not.

So for above script we have:
- fallocate
- buffered write
  If we don't have enough data space, we fall back to NOCOW check.
  At this timming, the extent is not shared, we can skip data
  reservation.
- reflink
  Now part of the large preallocated extent is shared.
- delalloc kicks in
  For the NOCOW range, as the preallocated extent is shared, we need
  to fall back to COW.

[WORKAROUND]
The workaround is to ensure any buffered write in the related extents
(not the reflink source range) get flushed before reflink.

However it's pretty expensive to do a comprehensive check.
In the reproducer, the reflink source is just a part of a larger
preallocated extent, we need to flush all buffered write of that extent
before reflink.
Such backward search can be complex and we may not get much benefit from
it.

So this patch will just try to flush the whole inode before reflink.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Reason for RFC:
Flushing an inode just because it's a reflink source is definitely
overkilling, but I don't have any better way to handle it.

Any comment on this is welcomed.
---
 fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7755b503b348..8caa0edb6fbf 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
 			return ret;
 	}
 
+	/*
+	 * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
+	 *
+	 * Due to the limit of btrfs extent tree design, we can only have
+	 * extent level share view. Any part of an extent is shared then the
+	 * whole extent is shared and any write into that extent needs to fall
+	 * back to COW.
+	 *
+	 * NOCOW buffered write without data space reserved could to lead to
+	 * either data space bytes_may_use underflow (kernel warning) or ENOSPC
+	 * at delalloc time (transaction abort).
+	 *
+	 * Here we take a shortcut by flush the whole inode. We could do better
+	 * by finding all extents in that range and flush the space referring
+	 * all those extents.
+	 * But that's too complex for such corner case.
+	 */
+	filemap_flush(src->i_mapping);
+	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
+		     &BTRFS_I(src)->runtime_flags))
+		filemap_flush(src->i_mapping);
+
 	/*
 	 * Lock destination range to serialize with concurrent readpages() and
 	 * source range to serialize with relocation.
-- 
2.21.0

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Filipe Manana]

On Fri, May 3, 2019 at 2:46 AM Qu Wenruo <wqu@suse.com> wrote:

What a great subject. The "reflink:" part is unnecessary, since the
rest of the subject already mentions it, that makes it a bit shorter.

>
> [BUG]
> The following command can lead to unexpected data COW:
>
>   #!/bin/bash
>
>   dev=/dev/test/test
>   mnt=/mnt/btrfs
>
>   mkfs.btrfs -f $dev -b 1G > /dev/null
>   mount $dev $mnt -o nospace_cache
>
>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>   umount $dev
>
> The result extent will be
>
>         item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
>                 generation 6 type 2 (prealloc)
>                 prealloc data disk byte 13631488 nr 28672
>         item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
>                 generation 6 type 1 (regular)
>                 extent data disk byte 13660160 nr 12288 <<< COW
>         item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
>                 generation 6 type 2 (prealloc)
>                 prealloc data disk byte 13631488 nr 28672
>
> Currently we always reserve space even for NOCOW buffered write, thus

I would add 'data' between 'reserve' and 'space', to be clear.

> under most case it shouldn't cause anything wrong even we fall back to
> COW.

even we ... -> even if we fallback to COW when running delalloc /
starting writeback.

>
> However when we're out of data space, we fall back to skip data space if
> we can do NOCOW write.

we fall back to skip data space ... -> we fallback to NOCOW write
without reserving data space.

>
> If such behavior happens under that case, we could hit the following
> problems:

> - data space bytes_may_use underflow
>   This will cause kernel warning.

Ok.

>
> - ENOSPC at delalloc time

at delalloc time - that is an ambiguous term you use through the change log.
You mean when running/starting delalloc, which happens when starting writeback,
but that could be confused with creating delalloc, which happens
during the buffered write path.

So I would always replace 'dealloc time' with 'when running delalloc'
(or when starting writeback).

>   This will lead to transaction abort and fs forced to RO.

Where does that happen exactly?
I don't recall starting transactions when running dealloc, and failed
to see where after a quick glance to cow_file_range()
and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
when starting writeback.

>
> [CAUSE]
> This is due to the fact that btrfs can only do extent level share check.
>
> Btrfs can only tell if an extent is shared, no matter if only part of the
> extent is shared or not.
>
> So for above script we have:
> - fallocate
> - buffered write
>   If we don't have enough data space, we fall back to NOCOW check.
>   At this timming, the extent is not shared, we can skip data
>   reservation.

But in the above example we don't fall to nocow mode when doing the
buffered write, as there's plenty of data space available (1Gb -
24Kb).
You need to update the example.


> - reflink
>   Now part of the large preallocated extent is shared.
> - delalloc kicks in

writeback kicks in

>   For the NOCOW range, as the preallocated extent is shared, we need
>   to fall back to COW.
>
> [WORKAROUND]
> The workaround is to ensure any buffered write in the related extents
> (not the reflink source range) get flushed before reflink.

not the reflink source range -> not just the reflink source range

>
> However it's pretty expensive to do a comprehensive check.
> In the reproducer, the reflink source is just a part of a larger

Again, the reproducer needs to be fixed (yes, I tested it even if it's
clear by looking at it that it doesn't trigger the nocow case).

> preallocated extent, we need to flush all buffered write of that extent
> before reflink.
> Such backward search can be complex and we may not get much benefit from
> it.
>
> So this patch will just try to flush the whole inode before reflink.


>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
> Flushing an inode just because it's a reflink source is definitely
> overkilling, but I don't have any better way to handle it.
>
> Any comment on this is welcomed.
> ---
>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7755b503b348..8caa0edb6fbf 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>                         return ret;
>         }
>
> +       /*
> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> +        *
> +        * Due to the limit of btrfs extent tree design, we can only have
> +        * extent level share view. Any part of an extent is shared then the

Any -> If any

> +        * whole extent is shared and any write into that extent needs to fall

is -> is considered

> +        * back to COW.

I would add, something like:

That is, btrfs' back references do not have a block level granularity,
they work at the whole extent level.

> +        *
> +        * NOCOW buffered write without data space reserved could to lead to
> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> +        * at delalloc time (transaction abort).

I would omit the warning and transaction abort parts, that can change
any time. And we have that information in the changelog, so it's not
lost.

> +        *
> +        * Here we take a shortcut by flush the whole inode. We could do better
> +        * by finding all extents in that range and flush the space referring
> +        * all those extents.
> +        * But that's too complex for such corner case.
> +        */
> +       filemap_flush(src->i_mapping);
> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> +                    &BTRFS_I(src)->runtime_flags))
> +               filemap_flush(src->i_mapping);

So a few comments here:

- why just in the clone part? The dedupe side has the same problem, doesn't it?

- I would move such flushing to btrfs_remap_file_range_prep - this is
where we do the source and target range flush and wait.

Can you turn the reproducer into an fstests case?

Thanks.

> +
>         /*
>          * Lock destination range to serialize with concurrent readpages() and
>          * source range to serialize with relocation.
> --
> 2.21.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 8018 bytes --]



On 2019/5/3 下午5:21, Filipe Manana wrote:
> On Fri, May 3, 2019 at 2:46 AM Qu Wenruo <wqu@suse.com> wrote:
> 
> What a great subject. The "reflink:" part is unnecessary, since the
> rest of the subject already mentions it, that makes it a bit shorter.
> 
>>
>> [BUG]
>> The following command can lead to unexpected data COW:
>>
>>   #!/bin/bash
>>
>>   dev=/dev/test/test
>>   mnt=/mnt/btrfs
>>
>>   mkfs.btrfs -f $dev -b 1G > /dev/null
>>   mount $dev $mnt -o nospace_cache
>>
>>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>>   umount $dev
>>
>> The result extent will be
>>
>>         item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
>>                 generation 6 type 2 (prealloc)
>>                 prealloc data disk byte 13631488 nr 28672
>>         item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
>>                 generation 6 type 1 (regular)
>>                 extent data disk byte 13660160 nr 12288 <<< COW
>>         item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
>>                 generation 6 type 2 (prealloc)
>>                 prealloc data disk byte 13631488 nr 28672
>>
>> Currently we always reserve space even for NOCOW buffered write, thus
> 
> I would add 'data' between 'reserve' and 'space', to be clear.
> 
>> under most case it shouldn't cause anything wrong even we fall back to
>> COW.
> 
> even we ... -> even if we fallback to COW when running delalloc /
> starting writeback.
> 
>>
>> However when we're out of data space, we fall back to skip data space if
>> we can do NOCOW write.
> 
> we fall back to skip data space ... -> we fallback to NOCOW write
> without reserving data space.
> 
>>
>> If such behavior happens under that case, we could hit the following
>> problems:
> 
>> - data space bytes_may_use underflow
>>   This will cause kernel warning.
> 
> Ok.
> 
>>
>> - ENOSPC at delalloc time
> 
> at delalloc time - that is an ambiguous term you use through the change log.

In fact, I have a lot of uncertain terminology through kernel.

Things like flush get referred multiple times in different context (e.g.
filemap flush, flushoncommit, super block flush).

If we have a terminology list, we can't be more happy to follow.

> You mean when running/starting delalloc, which happens when starting writeback,
> but that could be confused with creating delalloc, which happens
> during the buffered write path.

Another confusion due to different terminology.

My understanding of the write path is:
buffered write -> delalloc (start delalloc) -> ordered extent -> finish
ordered io.

Thus I missed the delalloc creating part.

> 
> So I would always replace 'dealloc time' with 'when running delalloc'
> (or when starting writeback).

I will change use running delalloc, with extra comment reference to the
function name (btrfs_run_delalloc_range()).

> 
>>   This will lead to transaction abort and fs forced to RO.
> 
> Where does that happen exactly?
My bad, I got confused with metadata writeback path.

For data writeback, it should only cause sync related failure.

> I don't recall starting transactions when running dealloc, and failed
> to see where after a quick glance to cow_file_range()
> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
> when starting writeback.
> 
>>
>> [CAUSE]
>> This is due to the fact that btrfs can only do extent level share check.
>>
>> Btrfs can only tell if an extent is shared, no matter if only part of the
>> extent is shared or not.
>>
>> So for above script we have:
>> - fallocate
>> - buffered write
>>   If we don't have enough data space, we fall back to NOCOW check.
>>   At this timming, the extent is not shared, we can skip data
>>   reservation.
> 
> But in the above example we don't fall to nocow mode when doing the
> buffered write, as there's plenty of data space available (1Gb -
> 24Kb).
> You need to update the example.
I have to admit that the core part is mostly based on the worst case
*assumption*.

I'll try to make the case convincing by making it fail directly.

> 
> 
>> - reflink
>>   Now part of the large preallocated extent is shared.
>> - delalloc kicks in
> 
> writeback kicks in
> 
>>   For the NOCOW range, as the preallocated extent is shared, we need
>>   to fall back to COW.
>>
>> [WORKAROUND]
>> The workaround is to ensure any buffered write in the related extents
>> (not the reflink source range) get flushed before reflink.
> 
> not the reflink source range -> not just the reflink source range
> 
>>
>> However it's pretty expensive to do a comprehensive check.
>> In the reproducer, the reflink source is just a part of a larger
> 
> Again, the reproducer needs to be fixed (yes, I tested it even if it's
> clear by looking at it that it doesn't trigger the nocow case).
> 
>> preallocated extent, we need to flush all buffered write of that extent
>> before reflink.
>> Such backward search can be complex and we may not get much benefit from
>> it.
>>
>> So this patch will just try to flush the whole inode before reflink.
> 
> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Reason for RFC:
>> Flushing an inode just because it's a reflink source is definitely
>> overkilling, but I don't have any better way to handle it.
>>
>> Any comment on this is welcomed.
>> ---
>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 7755b503b348..8caa0edb6fbf 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>                         return ret;
>>         }
>>
>> +       /*
>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>> +        *
>> +        * Due to the limit of btrfs extent tree design, we can only have
>> +        * extent level share view. Any part of an extent is shared then the
> 
> Any -> If any
> 
>> +        * whole extent is shared and any write into that extent needs to fall
> 
> is -> is considered
> 
>> +        * back to COW.
> 
> I would add, something like:
> 
> That is, btrfs' back references do not have a block level granularity,
> they work at the whole extent level.
> 
>> +        *
>> +        * NOCOW buffered write without data space reserved could to lead to
>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>> +        * at delalloc time (transaction abort).
> 
> I would omit the warning and transaction abort parts, that can change
> any time. And we have that information in the changelog, so it's not
> lost.
> 
>> +        *
>> +        * Here we take a shortcut by flush the whole inode. We could do better
>> +        * by finding all extents in that range and flush the space referring
>> +        * all those extents.
>> +        * But that's too complex for such corner case.
>> +        */
>> +       filemap_flush(src->i_mapping);
>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>> +                    &BTRFS_I(src)->runtime_flags))
>> +               filemap_flush(src->i_mapping);
> 
> So a few comments here:
> 
> - why just in the clone part? The dedupe side has the same problem, doesn't it?

Right.

> 
> - I would move such flushing to btrfs_remap_file_range_prep - this is
> where we do the source and target range flush and wait.
> 
> Can you turn the reproducer into an fstests case?

Sure.

Thanks for the info and all the comment,
Qu

> 
> Thanks.
> 
>> +
>>         /*
>>          * Lock destination range to serialize with concurrent readpages() and
>>          * source range to serialize with relocation.
>> --
>> 2.21.0
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Filipe Manana]

On Fri, May 3, 2019 at 11:18 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2019/5/3 下午5:21, Filipe Manana wrote:
> > On Fri, May 3, 2019 at 2:46 AM Qu Wenruo <wqu@suse.com> wrote:
> >
> > What a great subject. The "reflink:" part is unnecessary, since the
> > rest of the subject already mentions it, that makes it a bit shorter.
> >
> >>
> >> [BUG]
> >> The following command can lead to unexpected data COW:
> >>
> >>   #!/bin/bash
> >>
> >>   dev=/dev/test/test
> >>   mnt=/mnt/btrfs
> >>
> >>   mkfs.btrfs -f $dev -b 1G > /dev/null
> >>   mount $dev $mnt -o nospace_cache
> >>
> >>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
> >>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
> >>   umount $dev
> >>
> >> The result extent will be
> >>
> >>         item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
> >>                 generation 6 type 2 (prealloc)
> >>                 prealloc data disk byte 13631488 nr 28672
> >>         item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
> >>                 generation 6 type 1 (regular)
> >>                 extent data disk byte 13660160 nr 12288 <<< COW
> >>         item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
> >>                 generation 6 type 2 (prealloc)
> >>                 prealloc data disk byte 13631488 nr 28672
> >>
> >> Currently we always reserve space even for NOCOW buffered write, thus
> >
> > I would add 'data' between 'reserve' and 'space', to be clear.
> >
> >> under most case it shouldn't cause anything wrong even we fall back to
> >> COW.
> >
> > even we ... -> even if we fallback to COW when running delalloc /
> > starting writeback.
> >
> >>
> >> However when we're out of data space, we fall back to skip data space if
> >> we can do NOCOW write.
> >
> > we fall back to skip data space ... -> we fallback to NOCOW write
> > without reserving data space.
> >
> >>
> >> If such behavior happens under that case, we could hit the following
> >> problems:
> >
> >> - data space bytes_may_use underflow
> >>   This will cause kernel warning.
> >
> > Ok.
> >
> >>
> >> - ENOSPC at delalloc time
> >
> > at delalloc time - that is an ambiguous term you use through the change log.
>
> In fact, I have a lot of uncertain terminology through kernel.
>
> Things like flush get referred multiple times in different context (e.g.
> filemap flush, flushoncommit, super block flush).
>
> If we have a terminology list, we can't be more happy to follow.

So, some is kernel wide while others are btrfs specific.

A buffered write creates dealloc - copies data to pages, marks the
pages as dirty and tags the range in the extent io tree as dellaloc.
Running delalloc, flushes writeback (starts IO for the dirty pages and
tags them as under writeback) and does other necessary things (like
reserving extents).
When writeback finishes, we add a task to a work queue to run
btrfs_finish_ordered_io - after that happens we say that the ordered
extent completed.

It can get ambiguous very often.

>
> > You mean when running/starting delalloc, which happens when starting writeback,
> > but that could be confused with creating delalloc, which happens
> > during the buffered write path.
>
> Another confusion due to different terminology.
>
> My understanding of the write path is:
> buffered write -> delalloc (start delalloc) -> ordered extent -> finish
> ordered io.
>
> Thus I missed the delalloc creating part.
>
> >
> > So I would always replace 'dealloc time' with 'when running delalloc'
> > (or when starting writeback).
>
> I will change use running delalloc, with extra comment reference to the
> function name (btrfs_run_delalloc_range()).
>
> >
> >>   This will lead to transaction abort and fs forced to RO.
> >
> > Where does that happen exactly?
> My bad, I got confused with metadata writeback path.
>
> For data writeback, it should only cause sync related failure.

Ok, so please remove the transaction abort comments for next iteration.
By sync related failure, you mean running dealloc fails with ENOSPC,
since when it tries to reserve a data extent it fails as it can't find
any free extent.

>
> > I don't recall starting transactions when running dealloc, and failed
> > to see where after a quick glance to cow_file_range()
> > and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
> > when starting writeback.
> >
> >>
> >> [CAUSE]
> >> This is due to the fact that btrfs can only do extent level share check.
> >>
> >> Btrfs can only tell if an extent is shared, no matter if only part of the
> >> extent is shared or not.
> >>
> >> So for above script we have:
> >> - fallocate
> >> - buffered write
> >>   If we don't have enough data space, we fall back to NOCOW check.
> >>   At this timming, the extent is not shared, we can skip data
> >>   reservation.
> >
> > But in the above example we don't fall to nocow mode when doing the
> > buffered write, as there's plenty of data space available (1Gb -
> > 24Kb).
> > You need to update the example.
> I have to admit that the core part is mostly based on the worst case
> *assumption*.
>
> I'll try to make the case convincing by making it fail directly.

Great, thanks.

>
> >
> >
> >> - reflink
> >>   Now part of the large preallocated extent is shared.
> >> - delalloc kicks in
> >
> > writeback kicks in
> >
> >>   For the NOCOW range, as the preallocated extent is shared, we need
> >>   to fall back to COW.
> >>
> >> [WORKAROUND]
> >> The workaround is to ensure any buffered write in the related extents
> >> (not the reflink source range) get flushed before reflink.
> >
> > not the reflink source range -> not just the reflink source range
> >
> >>
> >> However it's pretty expensive to do a comprehensive check.
> >> In the reproducer, the reflink source is just a part of a larger
> >
> > Again, the reproducer needs to be fixed (yes, I tested it even if it's
> > clear by looking at it that it doesn't trigger the nocow case).
> >
> >> preallocated extent, we need to flush all buffered write of that extent
> >> before reflink.
> >> Such backward search can be complex and we may not get much benefit from
> >> it.
> >>
> >> So this patch will just try to flush the whole inode before reflink.
> >
> >
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> Reason for RFC:
> >> Flushing an inode just because it's a reflink source is definitely
> >> overkilling, but I don't have any better way to handle it.
> >>
> >> Any comment on this is welcomed.
> >> ---
> >>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> index 7755b503b348..8caa0edb6fbf 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> >>                         return ret;
> >>         }
> >>
> >> +       /*
> >> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> >> +        *
> >> +        * Due to the limit of btrfs extent tree design, we can only have
> >> +        * extent level share view. Any part of an extent is shared then the
> >
> > Any -> If any
> >
> >> +        * whole extent is shared and any write into that extent needs to fall
> >
> > is -> is considered
> >
> >> +        * back to COW.
> >
> > I would add, something like:
> >
> > That is, btrfs' back references do not have a block level granularity,
> > they work at the whole extent level.
> >
> >> +        *
> >> +        * NOCOW buffered write without data space reserved could to lead to
> >> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> >> +        * at delalloc time (transaction abort).
> >
> > I would omit the warning and transaction abort parts, that can change
> > any time. And we have that information in the changelog, so it's not
> > lost.
> >
> >> +        *
> >> +        * Here we take a shortcut by flush the whole inode. We could do better
> >> +        * by finding all extents in that range and flush the space referring
> >> +        * all those extents.
> >> +        * But that's too complex for such corner case.
> >> +        */
> >> +       filemap_flush(src->i_mapping);
> >> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> >> +                    &BTRFS_I(src)->runtime_flags))
> >> +               filemap_flush(src->i_mapping);
> >
> > So a few comments here:
> >
> > - why just in the clone part? The dedupe side has the same problem, doesn't it?
>
> Right.
>
> >
> > - I would move such flushing to btrfs_remap_file_range_prep - this is
> > where we do the source and target range flush and wait.
> >
> > Can you turn the reproducer into an fstests case?
>
> Sure.
>
> Thanks for the info and all the comment,
> Qu
>
> >
> > Thanks.
> >
> >> +
> >>         /*
> >>          * Lock destination range to serialize with concurrent readpages() and
> >>          * source range to serialize with relocation.
> >> --
> >> 2.21.0
> >>
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Nikolay Borisov]

On 3.05.19 г. 13:45 ч., Filipe Manana wrote:
> On Fri, May 3, 2019 at 11:18 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2019/5/3 下午5:21, Filipe Manana wrote:
>>> On Fri, May 3, 2019 at 2:46 AM Qu Wenruo <wqu@suse.com> wrote:
>>>
>>> What a great subject. The "reflink:" part is unnecessary, since the
>>> rest of the subject already mentions it, that makes it a bit shorter.
>>>
>>>>
>>>> [BUG]
>>>> The following command can lead to unexpected data COW:
>>>>
>>>>   #!/bin/bash
>>>>
>>>>   dev=/dev/test/test
>>>>   mnt=/mnt/btrfs
>>>>
>>>>   mkfs.btrfs -f $dev -b 1G > /dev/null
>>>>   mount $dev $mnt -o nospace_cache
>>>>
>>>>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>>>>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>>>>   umount $dev
>>>>
>>>> The result extent will be
>>>>
>>>>         item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
>>>>                 generation 6 type 2 (prealloc)
>>>>                 prealloc data disk byte 13631488 nr 28672
>>>>         item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
>>>>                 generation 6 type 1 (regular)
>>>>                 extent data disk byte 13660160 nr 12288 <<< COW
>>>>         item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
>>>>                 generation 6 type 2 (prealloc)
>>>>                 prealloc data disk byte 13631488 nr 28672
>>>>
>>>> Currently we always reserve space even for NOCOW buffered write, thus
>>>
>>> I would add 'data' between 'reserve' and 'space', to be clear.
>>>
>>>> under most case it shouldn't cause anything wrong even we fall back to
>>>> COW.
>>>
>>> even we ... -> even if we fallback to COW when running delalloc /
>>> starting writeback.
>>>
>>>>
>>>> However when we're out of data space, we fall back to skip data space if
>>>> we can do NOCOW write.
>>>
>>> we fall back to skip data space ... -> we fallback to NOCOW write
>>> without reserving data space.
>>>
>>>>
>>>> If such behavior happens under that case, we could hit the following
>>>> problems:
>>>
>>>> - data space bytes_may_use underflow
>>>>   This will cause kernel warning.
>>>
>>> Ok.
>>>
>>>>
>>>> - ENOSPC at delalloc time
>>>
>>> at delalloc time - that is an ambiguous term you use through the change log.
>>
>> In fact, I have a lot of uncertain terminology through kernel.
>>
>> Things like flush get referred multiple times in different context (e.g.
>> filemap flush, flushoncommit, super block flush).
>>
>> If we have a terminology list, we can't be more happy to follow.
> 
> So, some is kernel wide while others are btrfs specific.
> 
> A buffered write creates dealloc - copies data to pages, marks the
> pages as dirty and tags the range in the extent io tree as dellaloc.
> Running delalloc, flushes writeback (starts IO for the dirty pages and
> tags them as under writeback) and does other necessary things (like
> reserving extents).
> When writeback finishes, we add a task to a work queue to run
> btrfs_finish_ordered_io - after that happens we say that the ordered
> extent completed.
> 
> It can get ambiguous very often.


That's why I have created the following document which (tries) to
explain this:

https://github.com/btrfs/btrfs-dev-docs/blob/master/delalloc.txt

It's not perfect but it's better than nothing, feel free to contribute
improvements.

< snip>

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6930 bytes --]

[snip]
>>
>> For data writeback, it should only cause sync related failure.
> 
> Ok, so please remove the transaction abort comments for next iteration.
> By sync related failure, you mean running dealloc fails with ENOSPC,
> since when it tries to reserve a data extent it fails as it can't find
> any free extent.

Well, btrfs has some hidden way to fix such problem by itself already.

At buffered write time, we have the following call chain:
btrfs_buffered_write()
|- btrfs_check_data_free_space()
   |- btrfs_alloc_data_chunk_ondemand()
      |- need_commit = 2; /* We have 2 chance to retry, 1 to flush */
      |- do_chunk_alloc() /* we have no data space available */
      |- if (ret < 0 && ret == -ENOSPC)
      |-     goto commit_trans;  /* try to free some space */
      |- commit_trans:
      |-     need_commit--;
      |-     if (need_commit > 0) {
      |-         btrfs_start_delalloc_roots();
      |-         btrfs_wait_ordered_roots();
      |-     }

This means, as long as we hit ENOSPC for data space, we will start write
back, thus NODATACOW data will still hit disk as NODATACOW.

Such hidden behavior along with always-reserve-data-space solves the
problem pretty well.
We either:
- reserve data space
  Then no matter how it ends, we're OK, although it may end as CoW.

- Failed to reserve data space
  Writeback will be triggered anyway, no way to screw things around.

Thus this workaround has nothing to fix, but only make certain NODATACOW
reach disk as NODATACOW.

It makes some NODATACOW behaves more correctly but won't fix any obvious
bug.

My personal take is to fix any strange behavior even it won't cause any
problem, but the full inode writeback can be performance heavy.

So my question is, do we really need this anyway?

Thanks,
Qu

> 
>>
>>> I don't recall starting transactions when running dealloc, and failed
>>> to see where after a quick glance to cow_file_range()
>>> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
>>> when starting writeback.
>>>
>>>>
>>>> [CAUSE]
>>>> This is due to the fact that btrfs can only do extent level share check.
>>>>
>>>> Btrfs can only tell if an extent is shared, no matter if only part of the
>>>> extent is shared or not.
>>>>
>>>> So for above script we have:
>>>> - fallocate
>>>> - buffered write
>>>>   If we don't have enough data space, we fall back to NOCOW check.
>>>>   At this timming, the extent is not shared, we can skip data
>>>>   reservation.
>>>
>>> But in the above example we don't fall to nocow mode when doing the
>>> buffered write, as there's plenty of data space available (1Gb -
>>> 24Kb).
>>> You need to update the example.
>> I have to admit that the core part is mostly based on the worst case
>> *assumption*.
>>
>> I'll try to make the case convincing by making it fail directly.
> 
> Great, thanks.
> 
>>
>>>
>>>
>>>> - reflink
>>>>   Now part of the large preallocated extent is shared.
>>>> - delalloc kicks in
>>>
>>> writeback kicks in
>>>
>>>>   For the NOCOW range, as the preallocated extent is shared, we need
>>>>   to fall back to COW.
>>>>
>>>> [WORKAROUND]
>>>> The workaround is to ensure any buffered write in the related extents
>>>> (not the reflink source range) get flushed before reflink.
>>>
>>> not the reflink source range -> not just the reflink source range
>>>
>>>>
>>>> However it's pretty expensive to do a comprehensive check.
>>>> In the reproducer, the reflink source is just a part of a larger
>>>
>>> Again, the reproducer needs to be fixed (yes, I tested it even if it's
>>> clear by looking at it that it doesn't trigger the nocow case).
>>>
>>>> preallocated extent, we need to flush all buffered write of that extent
>>>> before reflink.
>>>> Such backward search can be complex and we may not get much benefit from
>>>> it.
>>>>
>>>> So this patch will just try to flush the whole inode before reflink.
>>>
>>>
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>> Reason for RFC:
>>>> Flushing an inode just because it's a reflink source is definitely
>>>> overkilling, but I don't have any better way to handle it.
>>>>
>>>> Any comment on this is welcomed.
>>>> ---
>>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index 7755b503b348..8caa0edb6fbf 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>>>                         return ret;
>>>>         }
>>>>
>>>> +       /*
>>>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>>>> +        *
>>>> +        * Due to the limit of btrfs extent tree design, we can only have
>>>> +        * extent level share view. Any part of an extent is shared then the
>>>
>>> Any -> If any
>>>
>>>> +        * whole extent is shared and any write into that extent needs to fall
>>>
>>> is -> is considered
>>>
>>>> +        * back to COW.
>>>
>>> I would add, something like:
>>>
>>> That is, btrfs' back references do not have a block level granularity,
>>> they work at the whole extent level.
>>>
>>>> +        *
>>>> +        * NOCOW buffered write without data space reserved could to lead to
>>>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>>>> +        * at delalloc time (transaction abort).
>>>
>>> I would omit the warning and transaction abort parts, that can change
>>> any time. And we have that information in the changelog, so it's not
>>> lost.
>>>
>>>> +        *
>>>> +        * Here we take a shortcut by flush the whole inode. We could do better
>>>> +        * by finding all extents in that range and flush the space referring
>>>> +        * all those extents.
>>>> +        * But that's too complex for such corner case.
>>>> +        */
>>>> +       filemap_flush(src->i_mapping);
>>>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>>>> +                    &BTRFS_I(src)->runtime_flags))
>>>> +               filemap_flush(src->i_mapping);
>>>
>>> So a few comments here:
>>>
>>> - why just in the clone part? The dedupe side has the same problem, doesn't it?
>>
>> Right.
>>
>>>
>>> - I would move such flushing to btrfs_remap_file_range_prep - this is
>>> where we do the source and target range flush and wait.
>>>
>>> Can you turn the reproducer into an fstests case?
>>
>> Sure.
>>
>> Thanks for the info and all the comment,
>> Qu
>>
>>>
>>> Thanks.
>>>
>>>> +
>>>>         /*
>>>>          * Lock destination range to serialize with concurrent readpages() and
>>>>          * source range to serialize with relocation.
>>>> --
>>>> 2.21.0
>>>>
>>>
>>>
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Nikolay Borisov]

On 6.05.19 г. 5:04 ч., Qu Wenruo wrote:
> [snip]
>>>
>>> For data writeback, it should only cause sync related failure.
>>
>> Ok, so please remove the transaction abort comments for next iteration.
>> By sync related failure, you mean running dealloc fails with ENOSPC,
>> since when it tries to reserve a data extent it fails as it can't find
>> any free extent.
> 
> Well, btrfs has some hidden way to fix such problem by itself already.
> 
> At buffered write time, we have the following call chain:
> btrfs_buffered_write()
> |- btrfs_check_data_free_space()
>    |- btrfs_alloc_data_chunk_ondemand()
>       |- need_commit = 2; /* We have 2 chance to retry, 1 to flush */
>       |- do_chunk_alloc() /* we have no data space available */
>       |- if (ret < 0 && ret == -ENOSPC)
>       |-     goto commit_trans;  /* try to free some space */
>       |- commit_trans:
>       |-     need_commit--;
>       |-     if (need_commit > 0) {
>       |-         btrfs_start_delalloc_roots();
>       |-         btrfs_wait_ordered_roots();
>       |-     }
> 
> This means, as long as we hit ENOSPC for data space, we will start write
> back, thus NODATACOW data will still hit disk as NODATACOW.

I'm lost for words at expressing how subtle and despicable that code is
... Is there a way to factor that out and make it more explicit ? I
don't think we should rely on such subtleties...

> 
> Such hidden behavior along with always-reserve-data-space solves the
> problem pretty well.
> We either:
> - reserve data space
>   Then no matter how it ends, we're OK, although it may end as CoW.
> 
> - Failed to reserve data space
>   Writeback will be triggered anyway, no way to screw things around.
> 
> Thus this workaround has nothing to fix, but only make certain NODATACOW
> reach disk as NODATACOW.
> 
> It makes some NODATACOW behaves more correctly but won't fix any obvious
> bug.
> 
> My personal take is to fix any strange behavior even it won't cause any
> problem, but the full inode writeback can be performance heavy.
> 
> So my question is, do we really need this anyway?
> 
> Thanks,
> Qu
> 
>>
>>>
>>>> I don't recall starting transactions when running dealloc, and failed
>>>> to see where after a quick glance to cow_file_range()
>>>> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
>>>> when starting writeback.
>>>>
>>>>>
>>>>> [CAUSE]
>>>>> This is due to the fact that btrfs can only do extent level share check.
>>>>>
>>>>> Btrfs can only tell if an extent is shared, no matter if only part of the
>>>>> extent is shared or not.
>>>>>
>>>>> So for above script we have:
>>>>> - fallocate
>>>>> - buffered write
>>>>>   If we don't have enough data space, we fall back to NOCOW check.
>>>>>   At this timming, the extent is not shared, we can skip data
>>>>>   reservation.
>>>>
>>>> But in the above example we don't fall to nocow mode when doing the
>>>> buffered write, as there's plenty of data space available (1Gb -
>>>> 24Kb).
>>>> You need to update the example.
>>> I have to admit that the core part is mostly based on the worst case
>>> *assumption*.
>>>
>>> I'll try to make the case convincing by making it fail directly.
>>
>> Great, thanks.
>>
>>>
>>>>
>>>>
>>>>> - reflink
>>>>>   Now part of the large preallocated extent is shared.
>>>>> - delalloc kicks in
>>>>
>>>> writeback kicks in
>>>>
>>>>>   For the NOCOW range, as the preallocated extent is shared, we need
>>>>>   to fall back to COW.
>>>>>
>>>>> [WORKAROUND]
>>>>> The workaround is to ensure any buffered write in the related extents
>>>>> (not the reflink source range) get flushed before reflink.
>>>>
>>>> not the reflink source range -> not just the reflink source range
>>>>
>>>>>
>>>>> However it's pretty expensive to do a comprehensive check.
>>>>> In the reproducer, the reflink source is just a part of a larger
>>>>
>>>> Again, the reproducer needs to be fixed (yes, I tested it even if it's
>>>> clear by looking at it that it doesn't trigger the nocow case).
>>>>
>>>>> preallocated extent, we need to flush all buffered write of that extent
>>>>> before reflink.
>>>>> Such backward search can be complex and we may not get much benefit from
>>>>> it.
>>>>>
>>>>> So this patch will just try to flush the whole inode before reflink.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>> Reason for RFC:
>>>>> Flushing an inode just because it's a reflink source is definitely
>>>>> overkilling, but I don't have any better way to handle it.
>>>>>
>>>>> Any comment on this is welcomed.
>>>>> ---
>>>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>> index 7755b503b348..8caa0edb6fbf 100644
>>>>> --- a/fs/btrfs/ioctl.c
>>>>> +++ b/fs/btrfs/ioctl.c
>>>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>>>>                         return ret;
>>>>>         }
>>>>>
>>>>> +       /*
>>>>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>>>>> +        *
>>>>> +        * Due to the limit of btrfs extent tree design, we can only have
>>>>> +        * extent level share view. Any part of an extent is shared then the
>>>>
>>>> Any -> If any
>>>>
>>>>> +        * whole extent is shared and any write into that extent needs to fall
>>>>
>>>> is -> is considered
>>>>
>>>>> +        * back to COW.
>>>>
>>>> I would add, something like:
>>>>
>>>> That is, btrfs' back references do not have a block level granularity,
>>>> they work at the whole extent level.
>>>>
>>>>> +        *
>>>>> +        * NOCOW buffered write without data space reserved could to lead to
>>>>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>>>>> +        * at delalloc time (transaction abort).
>>>>
>>>> I would omit the warning and transaction abort parts, that can change
>>>> any time. And we have that information in the changelog, so it's not
>>>> lost.
>>>>
>>>>> +        *
>>>>> +        * Here we take a shortcut by flush the whole inode. We could do better
>>>>> +        * by finding all extents in that range and flush the space referring
>>>>> +        * all those extents.
>>>>> +        * But that's too complex for such corner case.
>>>>> +        */
>>>>> +       filemap_flush(src->i_mapping);
>>>>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>>>>> +                    &BTRFS_I(src)->runtime_flags))
>>>>> +               filemap_flush(src->i_mapping);
>>>>
>>>> So a few comments here:
>>>>
>>>> - why just in the clone part? The dedupe side has the same problem, doesn't it?
>>>
>>> Right.
>>>
>>>>
>>>> - I would move such flushing to btrfs_remap_file_range_prep - this is
>>>> where we do the source and target range flush and wait.
>>>>
>>>> Can you turn the reproducer into an fstests case?
>>>
>>> Sure.
>>>
>>> Thanks for the info and all the comment,
>>> Qu
>>>
>>>>
>>>> Thanks.
>>>>
>>>>> +
>>>>>         /*
>>>>>          * Lock destination range to serialize with concurrent readpages() and
>>>>>          * source range to serialize with relocation.
>>>>> --
>>>>> 2.21.0
>>>>>
>>>>
>>>>
>>>
>>
>>
> 

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Filipe Manana]

On Mon, May 6, 2019 at 3:04 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
> [snip]
> >>
> >> For data writeback, it should only cause sync related failure.
> >
> > Ok, so please remove the transaction abort comments for next iteration.
> > By sync related failure, you mean running dealloc fails with ENOSPC,
> > since when it tries to reserve a data extent it fails as it can't find
> > any free extent.
>
> Well, btrfs has some hidden way to fix such problem by itself already.
>
> At buffered write time, we have the following call chain:
> btrfs_buffered_write()
> |- btrfs_check_data_free_space()
>    |- btrfs_alloc_data_chunk_ondemand()
>       |- need_commit = 2; /* We have 2 chance to retry, 1 to flush */
>       |- do_chunk_alloc() /* we have no data space available */
>       |- if (ret < 0 && ret == -ENOSPC)
>       |-     goto commit_trans;  /* try to free some space */
>       |- commit_trans:
>       |-     need_commit--;
>       |-     if (need_commit > 0) {
>       |-         btrfs_start_delalloc_roots();
>       |-         btrfs_wait_ordered_roots();
>       |-     }
>
> This means, as long as we hit ENOSPC for data space, we will start write
> back, thus NODATACOW data will still hit disk as NODATACOW.
>
> Such hidden behavior along with always-reserve-data-space solves the
> problem pretty well.

It doesn't solve the problem you reported in the rfc patch.

What happens:

1) We have a file with a prealloc extent, that isn't shared

2) We have 0 bytes of available data space (or any amount less then
that of the buffered write size)

3) A buffered write happens that falls within a subrange of the prealloc extent.
    We can't reserve space, we do all those things at
btrfs_alloc_data_chunk_ondemand(), but we can't get any data space
released, since it's all allocated.
    Therefore we fall back to nodatacow mode. We dirty the pages, mark
the range as dealloc, etc.

4) The reflink happens, for a subrange of the prealloc extent that
does not overlap the range of the buffered write.

5) Some time after the reflink, writeback starts for the inode.
    During the writeback we fallback to COW mode, because the prealloc
extent is shared, even if the subrange of the buffered write does not
overlap the reflinked subrange.
    Now the write silently fails with -ENOSPC, and a user doesn't know
about it unless it does an fsync after that writeback, which will
report the error via filemap_check_wb_err().

> We either:
> - reserve data space
>   Then no matter how it ends, we're OK, although it may end as CoW.
>
> - Failed to reserve data space
>   Writeback will be triggered anyway, no way to screw things around.
>
> Thus this workaround has nothing to fix, but only make certain NODATACOW
> reach disk as NODATACOW.
>
> It makes some NODATACOW behaves more correctly but won't fix any obvious
> bug.
>
> My personal take is to fix any strange behavior even it won't cause any
> problem, but the full inode writeback can be performance heavy.
>
> So my question is, do we really need this anyway?

Do we need what? Your patch, that logic at
btrfs_alloc_data_chunk_ondemand(), something else?

Thanks.

>
> Thanks,
> Qu
>
> >
> >>
> >>> I don't recall starting transactions when running dealloc, and failed
> >>> to see where after a quick glance to cow_file_range()
> >>> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
> >>> when starting writeback.
> >>>
> >>>>
> >>>> [CAUSE]
> >>>> This is due to the fact that btrfs can only do extent level share check.
> >>>>
> >>>> Btrfs can only tell if an extent is shared, no matter if only part of the
> >>>> extent is shared or not.
> >>>>
> >>>> So for above script we have:
> >>>> - fallocate
> >>>> - buffered write
> >>>>   If we don't have enough data space, we fall back to NOCOW check.
> >>>>   At this timming, the extent is not shared, we can skip data
> >>>>   reservation.
> >>>
> >>> But in the above example we don't fall to nocow mode when doing the
> >>> buffered write, as there's plenty of data space available (1Gb -
> >>> 24Kb).
> >>> You need to update the example.
> >> I have to admit that the core part is mostly based on the worst case
> >> *assumption*.
> >>
> >> I'll try to make the case convincing by making it fail directly.
> >
> > Great, thanks.
> >
> >>
> >>>
> >>>
> >>>> - reflink
> >>>>   Now part of the large preallocated extent is shared.
> >>>> - delalloc kicks in
> >>>
> >>> writeback kicks in
> >>>
> >>>>   For the NOCOW range, as the preallocated extent is shared, we need
> >>>>   to fall back to COW.
> >>>>
> >>>> [WORKAROUND]
> >>>> The workaround is to ensure any buffered write in the related extents
> >>>> (not the reflink source range) get flushed before reflink.
> >>>
> >>> not the reflink source range -> not just the reflink source range
> >>>
> >>>>
> >>>> However it's pretty expensive to do a comprehensive check.
> >>>> In the reproducer, the reflink source is just a part of a larger
> >>>
> >>> Again, the reproducer needs to be fixed (yes, I tested it even if it's
> >>> clear by looking at it that it doesn't trigger the nocow case).
> >>>
> >>>> preallocated extent, we need to flush all buffered write of that extent
> >>>> before reflink.
> >>>> Such backward search can be complex and we may not get much benefit from
> >>>> it.
> >>>>
> >>>> So this patch will just try to flush the whole inode before reflink.
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >>>> ---
> >>>> Reason for RFC:
> >>>> Flushing an inode just because it's a reflink source is definitely
> >>>> overkilling, but I don't have any better way to handle it.
> >>>>
> >>>> Any comment on this is welcomed.
> >>>> ---
> >>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
> >>>>  1 file changed, 22 insertions(+)
> >>>>
> >>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >>>> index 7755b503b348..8caa0edb6fbf 100644
> >>>> --- a/fs/btrfs/ioctl.c
> >>>> +++ b/fs/btrfs/ioctl.c
> >>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> >>>>                         return ret;
> >>>>         }
> >>>>
> >>>> +       /*
> >>>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> >>>> +        *
> >>>> +        * Due to the limit of btrfs extent tree design, we can only have
> >>>> +        * extent level share view. Any part of an extent is shared then the
> >>>
> >>> Any -> If any
> >>>
> >>>> +        * whole extent is shared and any write into that extent needs to fall
> >>>
> >>> is -> is considered
> >>>
> >>>> +        * back to COW.
> >>>
> >>> I would add, something like:
> >>>
> >>> That is, btrfs' back references do not have a block level granularity,
> >>> they work at the whole extent level.
> >>>
> >>>> +        *
> >>>> +        * NOCOW buffered write without data space reserved could to lead to
> >>>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> >>>> +        * at delalloc time (transaction abort).
> >>>
> >>> I would omit the warning and transaction abort parts, that can change
> >>> any time. And we have that information in the changelog, so it's not
> >>> lost.
> >>>
> >>>> +        *
> >>>> +        * Here we take a shortcut by flush the whole inode. We could do better
> >>>> +        * by finding all extents in that range and flush the space referring
> >>>> +        * all those extents.
> >>>> +        * But that's too complex for such corner case.
> >>>> +        */
> >>>> +       filemap_flush(src->i_mapping);
> >>>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> >>>> +                    &BTRFS_I(src)->runtime_flags))
> >>>> +               filemap_flush(src->i_mapping);
> >>>
> >>> So a few comments here:
> >>>
> >>> - why just in the clone part? The dedupe side has the same problem, doesn't it?
> >>
> >> Right.
> >>
> >>>
> >>> - I would move such flushing to btrfs_remap_file_range_prep - this is
> >>> where we do the source and target range flush and wait.
> >>>
> >>> Can you turn the reproducer into an fstests case?
> >>
> >> Sure.
> >>
> >> Thanks for the info and all the comment,
> >> Qu
> >>
> >>>
> >>> Thanks.
> >>>
> >>>> +
> >>>>         /*
> >>>>          * Lock destination range to serialize with concurrent readpages() and
> >>>>          * source range to serialize with relocation.
> >>>> --
> >>>> 2.21.0
> >>>>
> >>>
> >>>
> >>
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 10929 bytes --]



On 2019/5/7 下午4:56, Filipe Manana wrote:
> On Mon, May 6, 2019 at 3:04 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>> [snip]
>>>>
>>>> For data writeback, it should only cause sync related failure.
>>>
>>> Ok, so please remove the transaction abort comments for next iteration.
>>> By sync related failure, you mean running dealloc fails with ENOSPC,
>>> since when it tries to reserve a data extent it fails as it can't find
>>> any free extent.
>>
>> Well, btrfs has some hidden way to fix such problem by itself already.
>>
>> At buffered write time, we have the following call chain:
>> btrfs_buffered_write()
>> |- btrfs_check_data_free_space()
>>    |- btrfs_alloc_data_chunk_ondemand()
>>       |- need_commit = 2; /* We have 2 chance to retry, 1 to flush */
>>       |- do_chunk_alloc() /* we have no data space available */
>>       |- if (ret < 0 && ret == -ENOSPC)
>>       |-     goto commit_trans;  /* try to free some space */
>>       |- commit_trans:
>>       |-     need_commit--;
>>       |-     if (need_commit > 0) {
>>       |-         btrfs_start_delalloc_roots();
>>       |-         btrfs_wait_ordered_roots();
>>       |-     }
>>
>> This means, as long as we hit ENOSPC for data space, we will start write
>> back, thus NODATACOW data will still hit disk as NODATACOW.
>>
>> Such hidden behavior along with always-reserve-data-space solves the
>> problem pretty well.
> 
> It doesn't solve the problem you reported in the rfc patch.

You're right, it doesn't solve the problem at all.
In fact, another bug caused my test script to pass even with some dirty
pages unable to be flushed back.

But it at least make sure all other pages reach disk as NODATACOW except
the last page.

> 
> What happens:
> 
> 1) We have a file with a prealloc extent, that isn't shared
> 
> 2) We have 0 bytes of available data space (or any amount less then
> that of the buffered write size)
> 
> 3) A buffered write happens that falls within a subrange of the prealloc extent.
>     We can't reserve space, we do all those things at
> btrfs_alloc_data_chunk_ondemand(), but we can't get any data space
> released, since it's all allocated.

At that time, we're already flushing all previously buffered write data.

E.g. if we're writing into one 1M preallocated extent.
The first 4K, we have no data space reserved, dirtied the page, prepare
all delalloc.

Then the 2nd 4K, we call btrfs_check_data_free_space(), as we're at low
data free space already, we flush all inodes, including the previous 4K
we just dirtied.
Then the first 4K get written to disk NODATACOW, as expected.

This loop happens until we reach the last page.

>     Therefore we fall back to nodatacow mode. We dirty the pages, mark
> the range as dealloc, etc.
> 
> 4) The reflink happens, for a subrange of the prealloc extent that
> does not overlap the range of the buffered write.

Just before the reflink, we only have 1 dirty page (the last page of
that buffered write) doesn't reach disk yet.

For the final page, we have no choice but do COW, and it fails with -ENOSPC.

However due to some other problem, the -ENOSPC doesn't reach user space
at all.


> 
> 5) Some time after the reflink, writeback starts for the inode.
>     During the writeback we fallback to COW mode, because the prealloc
> extent is shared, even if the subrange of the buffered write does not
> overlap the reflinked subrange.
>     Now the write silently fails with -ENOSPC, and a user doesn't know
> about it unless it does an fsync after that writeback, which will
> report the error via filemap_check_wb_err().
> 
>> We either:
>> - reserve data space
>>   Then no matter how it ends, we're OK, although it may end as CoW.
>>
>> - Failed to reserve data space
>>   Writeback will be triggered anyway, no way to screw things around.
>>
>> Thus this workaround has nothing to fix, but only make certain NODATACOW
>> reach disk as NODATACOW.
>>
>> It makes some NODATACOW behaves more correctly but won't fix any obvious
>> bug.
>>
>> My personal take is to fix any strange behavior even it won't cause any
>> problem, but the full inode writeback can be performance heavy.
>>
>> So my question is, do we really need this anyway?
> 
> Do we need what? Your patch, that logic at
> btrfs_alloc_data_chunk_ondemand(), something else?

I meant the patch, but the deeper I dig into the problem, more problem I
found.

The patch is still needed, but there is a more important bug, that
btrfs_run_delalloc_range() failure won't be reported in sync.

The script here I'm using is:
------
#!/bin/bash

dev=/dev/test/test
mnt=/mnt/btrfs

#mkfs.btrfs -f $dev -b 1G > /dev/null
#mount $dev $mnt -o nospace_cache

umount $mnt &> /dev/null
umount $dev &> /dev/null

dmesg -C
mkfs.btrfs -f $dev -b 512M > /dev/null

mount $dev $mnt -o nospace_cache

xfs_io -f -c "falloc 8k 64m" $mnt/file1
xfs_io -f -c "pwrite 0 -b 4k 370M" $mnt/padding

sync
btrfs fi df $mnt --raw

xfs_io -c "pwrite 1m 16m" $mnt/file1
echo "nodatacow write finished" >> /dev/kmsg
xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
echo "reflink finished" >> /dev/kmsg
sync
echo "sync finished ret=$?" >> /dev/kmsg
umount $dev
------

As describe, the last write at 17821696 (17M - 4K) will fail due to ENOSPC.
But the sync succeeded without reporting any problem.

Thanks,
Qu

> 
> Thanks.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>>> I don't recall starting transactions when running dealloc, and failed
>>>>> to see where after a quick glance to cow_file_range()
>>>>> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
>>>>> when starting writeback.
>>>>>
>>>>>>
>>>>>> [CAUSE]
>>>>>> This is due to the fact that btrfs can only do extent level share check.
>>>>>>
>>>>>> Btrfs can only tell if an extent is shared, no matter if only part of the
>>>>>> extent is shared or not.
>>>>>>
>>>>>> So for above script we have:
>>>>>> - fallocate
>>>>>> - buffered write
>>>>>>   If we don't have enough data space, we fall back to NOCOW check.
>>>>>>   At this timming, the extent is not shared, we can skip data
>>>>>>   reservation.
>>>>>
>>>>> But in the above example we don't fall to nocow mode when doing the
>>>>> buffered write, as there's plenty of data space available (1Gb -
>>>>> 24Kb).
>>>>> You need to update the example.
>>>> I have to admit that the core part is mostly based on the worst case
>>>> *assumption*.
>>>>
>>>> I'll try to make the case convincing by making it fail directly.
>>>
>>> Great, thanks.
>>>
>>>>
>>>>>
>>>>>
>>>>>> - reflink
>>>>>>   Now part of the large preallocated extent is shared.
>>>>>> - delalloc kicks in
>>>>>
>>>>> writeback kicks in
>>>>>
>>>>>>   For the NOCOW range, as the preallocated extent is shared, we need
>>>>>>   to fall back to COW.
>>>>>>
>>>>>> [WORKAROUND]
>>>>>> The workaround is to ensure any buffered write in the related extents
>>>>>> (not the reflink source range) get flushed before reflink.
>>>>>
>>>>> not the reflink source range -> not just the reflink source range
>>>>>
>>>>>>
>>>>>> However it's pretty expensive to do a comprehensive check.
>>>>>> In the reproducer, the reflink source is just a part of a larger
>>>>>
>>>>> Again, the reproducer needs to be fixed (yes, I tested it even if it's
>>>>> clear by looking at it that it doesn't trigger the nocow case).
>>>>>
>>>>>> preallocated extent, we need to flush all buffered write of that extent
>>>>>> before reflink.
>>>>>> Such backward search can be complex and we may not get much benefit from
>>>>>> it.
>>>>>>
>>>>>> So this patch will just try to flush the whole inode before reflink.
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>>> ---
>>>>>> Reason for RFC:
>>>>>> Flushing an inode just because it's a reflink source is definitely
>>>>>> overkilling, but I don't have any better way to handle it.
>>>>>>
>>>>>> Any comment on this is welcomed.
>>>>>> ---
>>>>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>>>>>  1 file changed, 22 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>>> index 7755b503b348..8caa0edb6fbf 100644
>>>>>> --- a/fs/btrfs/ioctl.c
>>>>>> +++ b/fs/btrfs/ioctl.c
>>>>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>>>>>                         return ret;
>>>>>>         }
>>>>>>
>>>>>> +       /*
>>>>>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>>>>>> +        *
>>>>>> +        * Due to the limit of btrfs extent tree design, we can only have
>>>>>> +        * extent level share view. Any part of an extent is shared then the
>>>>>
>>>>> Any -> If any
>>>>>
>>>>>> +        * whole extent is shared and any write into that extent needs to fall
>>>>>
>>>>> is -> is considered
>>>>>
>>>>>> +        * back to COW.
>>>>>
>>>>> I would add, something like:
>>>>>
>>>>> That is, btrfs' back references do not have a block level granularity,
>>>>> they work at the whole extent level.
>>>>>
>>>>>> +        *
>>>>>> +        * NOCOW buffered write without data space reserved could to lead to
>>>>>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>>>>>> +        * at delalloc time (transaction abort).
>>>>>
>>>>> I would omit the warning and transaction abort parts, that can change
>>>>> any time. And we have that information in the changelog, so it's not
>>>>> lost.
>>>>>
>>>>>> +        *
>>>>>> +        * Here we take a shortcut by flush the whole inode. We could do better
>>>>>> +        * by finding all extents in that range and flush the space referring
>>>>>> +        * all those extents.
>>>>>> +        * But that's too complex for such corner case.
>>>>>> +        */
>>>>>> +       filemap_flush(src->i_mapping);
>>>>>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>>>>>> +                    &BTRFS_I(src)->runtime_flags))
>>>>>> +               filemap_flush(src->i_mapping);
>>>>>
>>>>> So a few comments here:
>>>>>
>>>>> - why just in the clone part? The dedupe side has the same problem, doesn't it?
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> - I would move such flushing to btrfs_remap_file_range_prep - this is
>>>>> where we do the source and target range flush and wait.
>>>>>
>>>>> Can you turn the reproducer into an fstests case?
>>>>
>>>> Sure.
>>>>
>>>> Thanks for the info and all the comment,
>>>> Qu
>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>>> +
>>>>>>         /*
>>>>>>          * Lock destination range to serialize with concurrent readpages() and
>>>>>>          * source range to serialize with relocation.
>>>>>> --
>>>>>> 2.21.0
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Filipe Manana]

On Tue, May 7, 2019 at 12:13 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2019/5/7 下午4:56, Filipe Manana wrote:
> > On Mon, May 6, 2019 at 3:04 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >>
> >> [snip]
> >>>>
> >>>> For data writeback, it should only cause sync related failure.
> >>>
> >>> Ok, so please remove the transaction abort comments for next iteration.
> >>> By sync related failure, you mean running dealloc fails with ENOSPC,
> >>> since when it tries to reserve a data extent it fails as it can't find
> >>> any free extent.
> >>
> >> Well, btrfs has some hidden way to fix such problem by itself already.
> >>
> >> At buffered write time, we have the following call chain:
> >> btrfs_buffered_write()
> >> |- btrfs_check_data_free_space()
> >>    |- btrfs_alloc_data_chunk_ondemand()
> >>       |- need_commit = 2; /* We have 2 chance to retry, 1 to flush */
> >>       |- do_chunk_alloc() /* we have no data space available */
> >>       |- if (ret < 0 && ret == -ENOSPC)
> >>       |-     goto commit_trans;  /* try to free some space */
> >>       |- commit_trans:
> >>       |-     need_commit--;
> >>       |-     if (need_commit > 0) {
> >>       |-         btrfs_start_delalloc_roots();
> >>       |-         btrfs_wait_ordered_roots();
> >>       |-     }
> >>
> >> This means, as long as we hit ENOSPC for data space, we will start write
> >> back, thus NODATACOW data will still hit disk as NODATACOW.
> >>
> >> Such hidden behavior along with always-reserve-data-space solves the
> >> problem pretty well.
> >
> > It doesn't solve the problem you reported in the rfc patch.
>
> You're right, it doesn't solve the problem at all.
> In fact, another bug caused my test script to pass even with some dirty
> pages unable to be flushed back.
>
> But it at least make sure all other pages reach disk as NODATACOW except
> the last page.
>
> >
> > What happens:
> >
> > 1) We have a file with a prealloc extent, that isn't shared
> >
> > 2) We have 0 bytes of available data space (or any amount less then
> > that of the buffered write size)
> >
> > 3) A buffered write happens that falls within a subrange of the prealloc extent.
> >     We can't reserve space, we do all those things at
> > btrfs_alloc_data_chunk_ondemand(), but we can't get any data space
> > released, since it's all allocated.
>
> At that time, we're already flushing all previously buffered write data.
>
> E.g. if we're writing into one 1M preallocated extent.
> The first 4K, we have no data space reserved, dirtied the page, prepare
> all delalloc.
>
> Then the 2nd 4K, we call btrfs_check_data_free_space(), as we're at low
> data free space already, we flush all inodes, including the previous 4K
> we just dirtied.
> Then the first 4K get written to disk NODATACOW, as expected.
>
> This loop happens until we reach the last page.

We fragment a buffered write into several chunks depending on its
size, the size of a page, the size of a pointer, etc.
We will hardly iterate for each page alone.
If your point is that btrfs_check_data_free_space() can be called
multiple times for a single buffered write, yes, that's true if it's
large than a certain threshold, but that will hardly be for each
individual page, typically it's much more than that.

>
> >     Therefore we fall back to nodatacow mode. We dirty the pages, mark
> > the range as dealloc, etc.
> >
> > 4) The reflink happens, for a subrange of the prealloc extent that
> > does not overlap the range of the buffered write.
>
> Just before the reflink, we only have 1 dirty page (the last page of
> that buffered write) doesn't reach disk yet.
>
> For the final page, we have no choice but do COW, and it fails with -ENOSPC.

Yes, isn't that what I said?

>
> However due to some other problem, the -ENOSPC doesn't reach user space
> at all.

Yes, isn't that what I said?

Yes... Same as the snapshotting case Robbie fixed. No news here.
Same as many other silent data loss issues I fixed with fsync a few
times sometime ago as well.

>
>
> >
> > 5) Some time after the reflink, writeback starts for the inode.
> >     During the writeback we fallback to COW mode, because the prealloc
> > extent is shared, even if the subrange of the buffered write does not
> > overlap the reflinked subrange.
> >     Now the write silently fails with -ENOSPC, and a user doesn't know
> > about it unless it does an fsync after that writeback, which will
> > report the error via filemap_check_wb_err().
> >
> >> We either:
> >> - reserve data space
> >>   Then no matter how it ends, we're OK, although it may end as CoW.
> >>
> >> - Failed to reserve data space
> >>   Writeback will be triggered anyway, no way to screw things around.
> >>
> >> Thus this workaround has nothing to fix, but only make certain NODATACOW
> >> reach disk as NODATACOW.
> >>
> >> It makes some NODATACOW behaves more correctly but won't fix any obvious
> >> bug.
> >>
> >> My personal take is to fix any strange behavior even it won't cause any
> >> problem, but the full inode writeback can be performance heavy.
> >>
> >> So my question is, do we really need this anyway?
> >
> > Do we need what? Your patch, that logic at
> > btrfs_alloc_data_chunk_ondemand(), something else?
>
> I meant the patch, but the deeper I dig into the problem, more problem I
> found.
>
> The patch is still needed, but there is a more important bug, that
> btrfs_run_delalloc_range() failure won't be reported in sync.

How could it report a failure?

sync calls sync(2), which returns void, meaning it always succeeds and
has no way to report errors.
See https://linux.die.net/man/2/sync

Error tracking and reporting was precisely one of main goals of
filemap_check_wb_err() and all the related work Jeff Layton did
sometime ago.

>
> The script here I'm using is:
> ------
> #!/bin/bash
>
> dev=/dev/test/test
> mnt=/mnt/btrfs
>
> #mkfs.btrfs -f $dev -b 1G > /dev/null
> #mount $dev $mnt -o nospace_cache
>
> umount $mnt &> /dev/null
> umount $dev &> /dev/null
>
> dmesg -C
> mkfs.btrfs -f $dev -b 512M > /dev/null
>
> mount $dev $mnt -o nospace_cache
>
> xfs_io -f -c "falloc 8k 64m" $mnt/file1
> xfs_io -f -c "pwrite 0 -b 4k 370M" $mnt/padding
>
> sync
> btrfs fi df $mnt --raw
>
> xfs_io -c "pwrite 1m 16m" $mnt/file1
> echo "nodatacow write finished" >> /dev/kmsg
> xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
> echo "reflink finished" >> /dev/kmsg
> sync
> echo "sync finished ret=$?" >> /dev/kmsg
> umount $dev
> ------
>
> As describe, the last write at 17821696 (17M - 4K) will fail due to ENOSPC.
> But the sync succeeded without reporting any problem.
>
> Thanks,
> Qu
>
> >
> > Thanks.
> >
> >>
> >> Thanks,
> >> Qu
> >>
> >>>
> >>>>
> >>>>> I don't recall starting transactions when running dealloc, and failed
> >>>>> to see where after a quick glance to cow_file_range()
> >>>>> and run_delalloc_nocow(). I'm assuming that 'at delalloc time' means
> >>>>> when starting writeback.
> >>>>>
> >>>>>>
> >>>>>> [CAUSE]
> >>>>>> This is due to the fact that btrfs can only do extent level share check.
> >>>>>>
> >>>>>> Btrfs can only tell if an extent is shared, no matter if only part of the
> >>>>>> extent is shared or not.
> >>>>>>
> >>>>>> So for above script we have:
> >>>>>> - fallocate
> >>>>>> - buffered write
> >>>>>>   If we don't have enough data space, we fall back to NOCOW check.
> >>>>>>   At this timming, the extent is not shared, we can skip data
> >>>>>>   reservation.
> >>>>>
> >>>>> But in the above example we don't fall to nocow mode when doing the
> >>>>> buffered write, as there's plenty of data space available (1Gb -
> >>>>> 24Kb).
> >>>>> You need to update the example.
> >>>> I have to admit that the core part is mostly based on the worst case
> >>>> *assumption*.
> >>>>
> >>>> I'll try to make the case convincing by making it fail directly.
> >>>
> >>> Great, thanks.
> >>>
> >>>>
> >>>>>
> >>>>>
> >>>>>> - reflink
> >>>>>>   Now part of the large preallocated extent is shared.
> >>>>>> - delalloc kicks in
> >>>>>
> >>>>> writeback kicks in
> >>>>>
> >>>>>>   For the NOCOW range, as the preallocated extent is shared, we need
> >>>>>>   to fall back to COW.
> >>>>>>
> >>>>>> [WORKAROUND]
> >>>>>> The workaround is to ensure any buffered write in the related extents
> >>>>>> (not the reflink source range) get flushed before reflink.
> >>>>>
> >>>>> not the reflink source range -> not just the reflink source range
> >>>>>
> >>>>>>
> >>>>>> However it's pretty expensive to do a comprehensive check.
> >>>>>> In the reproducer, the reflink source is just a part of a larger
> >>>>>
> >>>>> Again, the reproducer needs to be fixed (yes, I tested it even if it's
> >>>>> clear by looking at it that it doesn't trigger the nocow case).
> >>>>>
> >>>>>> preallocated extent, we need to flush all buffered write of that extent
> >>>>>> before reflink.
> >>>>>> Such backward search can be complex and we may not get much benefit from
> >>>>>> it.
> >>>>>>
> >>>>>> So this patch will just try to flush the whole inode before reflink.
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >>>>>> ---
> >>>>>> Reason for RFC:
> >>>>>> Flushing an inode just because it's a reflink source is definitely
> >>>>>> overkilling, but I don't have any better way to handle it.
> >>>>>>
> >>>>>> Any comment on this is welcomed.
> >>>>>> ---
> >>>>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
> >>>>>>  1 file changed, 22 insertions(+)
> >>>>>>
> >>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >>>>>> index 7755b503b348..8caa0edb6fbf 100644
> >>>>>> --- a/fs/btrfs/ioctl.c
> >>>>>> +++ b/fs/btrfs/ioctl.c
> >>>>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> >>>>>>                         return ret;
> >>>>>>         }
> >>>>>>
> >>>>>> +       /*
> >>>>>> +        * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> >>>>>> +        *
> >>>>>> +        * Due to the limit of btrfs extent tree design, we can only have
> >>>>>> +        * extent level share view. Any part of an extent is shared then the
> >>>>>
> >>>>> Any -> If any
> >>>>>
> >>>>>> +        * whole extent is shared and any write into that extent needs to fall
> >>>>>
> >>>>> is -> is considered
> >>>>>
> >>>>>> +        * back to COW.
> >>>>>
> >>>>> I would add, something like:
> >>>>>
> >>>>> That is, btrfs' back references do not have a block level granularity,
> >>>>> they work at the whole extent level.
> >>>>>
> >>>>>> +        *
> >>>>>> +        * NOCOW buffered write without data space reserved could to lead to
> >>>>>> +        * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> >>>>>> +        * at delalloc time (transaction abort).
> >>>>>
> >>>>> I would omit the warning and transaction abort parts, that can change
> >>>>> any time. And we have that information in the changelog, so it's not
> >>>>> lost.
> >>>>>
> >>>>>> +        *
> >>>>>> +        * Here we take a shortcut by flush the whole inode. We could do better
> >>>>>> +        * by finding all extents in that range and flush the space referring
> >>>>>> +        * all those extents.
> >>>>>> +        * But that's too complex for such corner case.
> >>>>>> +        */
> >>>>>> +       filemap_flush(src->i_mapping);
> >>>>>> +       if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> >>>>>> +                    &BTRFS_I(src)->runtime_flags))
> >>>>>> +               filemap_flush(src->i_mapping);
> >>>>>
> >>>>> So a few comments here:
> >>>>>
> >>>>> - why just in the clone part? The dedupe side has the same problem, doesn't it?
> >>>>
> >>>> Right.
> >>>>
> >>>>>
> >>>>> - I would move such flushing to btrfs_remap_file_range_prep - this is
> >>>>> where we do the source and target range flush and wait.
> >>>>>
> >>>>> Can you turn the reproducer into an fstests case?
> >>>>
> >>>> Sure.
> >>>>
> >>>> Thanks for the info and all the comment,
> >>>> Qu
> >>>>
> >>>>>
> >>>>> Thanks.
> >>>>>
> >>>>>> +
> >>>>>>         /*
> >>>>>>          * Lock destination range to serialize with concurrent readpages() and
> >>>>>>          * source range to serialize with relocation.
> >>>>>> --
> >>>>>> 2.21.0
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

On Fri, May 03, 2019 at 09:08:52AM +0800, Qu Wenruo wrote:
> [BUG]
> The following command can lead to unexpected data COW:
> 
>   #!/bin/bash
> 
>   dev=/dev/test/test
>   mnt=/mnt/btrfs
> 
>   mkfs.btrfs -f $dev -b 1G > /dev/null
>   mount $dev $mnt -o nospace_cache
> 
>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>   umount $dev
> 
> The result extent will be
> 
> 	item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
> 		generation 6 type 2 (prealloc)
> 		prealloc data disk byte 13631488 nr 28672
> 	item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
> 		generation 6 type 1 (regular)
> 		extent data disk byte 13660160 nr 12288 <<< COW
> 	item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
> 		generation 6 type 2 (prealloc)
> 		prealloc data disk byte 13631488 nr 28672
> 
> Currently we always reserve space even for NOCOW buffered write, thus
> under most case it shouldn't cause anything wrong even we fall back to
> COW.
> 
> However when we're out of data space, we fall back to skip data space if
> we can do NOCOW write.
> 
> If such behavior happens under that case, we could hit the following
> problems:
> - data space bytes_may_use underflow
>   This will cause kernel warning.
> 
> - ENOSPC at delalloc time
>   This will lead to transaction abort and fs forced to RO.
> 
> [CAUSE]
> This is due to the fact that btrfs can only do extent level share check.
> 
> Btrfs can only tell if an extent is shared, no matter if only part of the
> extent is shared or not.
> 
> So for above script we have:
> - fallocate
> - buffered write
>   If we don't have enough data space, we fall back to NOCOW check.
>   At this timming, the extent is not shared, we can skip data
>   reservation.
> - reflink
>   Now part of the large preallocated extent is shared.
> - delalloc kicks in
>   For the NOCOW range, as the preallocated extent is shared, we need
>   to fall back to COW.
> 
> [WORKAROUND]
> The workaround is to ensure any buffered write in the related extents
> (not the reflink source range) get flushed before reflink.
> 
> However it's pretty expensive to do a comprehensive check.
> In the reproducer, the reflink source is just a part of a larger
> preallocated extent, we need to flush all buffered write of that extent
> before reflink.
> Such backward search can be complex and we may not get much benefit from
> it.
> 
> So this patch will just try to flush the whole inode before reflink.

Does that mean that if a large file is being written and deduped
simultaneously, that the dedupes would now trigger flushes over the
entire file?  That seems like it could be slow.

e.g. if the file is a big VM image file and it is used src and for dedupe
(which is quite common in VM image files), we'd be hammering the disk
with writes similar to hitting it with fsync() in a tight loop?

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
> Flushing an inode just because it's a reflink source is definitely
> overkilling, but I don't have any better way to handle it.
> 
> Any comment on this is welcomed.
> ---
>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7755b503b348..8caa0edb6fbf 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>  			return ret;
>  	}
>  
> +	/*
> +	 * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> +	 *
> +	 * Due to the limit of btrfs extent tree design, we can only have
> +	 * extent level share view. Any part of an extent is shared then the
> +	 * whole extent is shared and any write into that extent needs to fall
> +	 * back to COW.
> +	 *
> +	 * NOCOW buffered write without data space reserved could to lead to
> +	 * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> +	 * at delalloc time (transaction abort).
> +	 *
> +	 * Here we take a shortcut by flush the whole inode. We could do better
> +	 * by finding all extents in that range and flush the space referring
> +	 * all those extents.
> +	 * But that's too complex for such corner case.
> +	 */
> +	filemap_flush(src->i_mapping);
> +	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> +		     &BTRFS_I(src)->runtime_flags))
> +		filemap_flush(src->i_mapping);
> +
>  	/*
>  	 * Lock destination range to serialize with concurrent readpages() and
>  	 * source range to serialize with relocation.
> -- 
> 2.21.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5206 bytes --]



On 2019/5/4 上午5:56, Zygo Blaxell wrote:
> On Fri, May 03, 2019 at 09:08:52AM +0800, Qu Wenruo wrote:
>> [BUG]
>> The following command can lead to unexpected data COW:
>>
>>   #!/bin/bash
>>
>>   dev=/dev/test/test
>>   mnt=/mnt/btrfs
>>
>>   mkfs.btrfs -f $dev -b 1G > /dev/null
>>   mount $dev $mnt -o nospace_cache
>>
>>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>>   umount $dev
>>
>> The result extent will be
>>
>> 	item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
>> 		generation 6 type 2 (prealloc)
>> 		prealloc data disk byte 13631488 nr 28672
>> 	item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
>> 		generation 6 type 1 (regular)
>> 		extent data disk byte 13660160 nr 12288 <<< COW
>> 	item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
>> 		generation 6 type 2 (prealloc)
>> 		prealloc data disk byte 13631488 nr 28672
>>
>> Currently we always reserve space even for NOCOW buffered write, thus
>> under most case it shouldn't cause anything wrong even we fall back to
>> COW.
>>
>> However when we're out of data space, we fall back to skip data space if
>> we can do NOCOW write.
>>
>> If such behavior happens under that case, we could hit the following
>> problems:
>> - data space bytes_may_use underflow
>>   This will cause kernel warning.
>>
>> - ENOSPC at delalloc time
>>   This will lead to transaction abort and fs forced to RO.
>>
>> [CAUSE]
>> This is due to the fact that btrfs can only do extent level share check.
>>
>> Btrfs can only tell if an extent is shared, no matter if only part of the
>> extent is shared or not.
>>
>> So for above script we have:
>> - fallocate
>> - buffered write
>>   If we don't have enough data space, we fall back to NOCOW check.
>>   At this timming, the extent is not shared, we can skip data
>>   reservation.
>> - reflink
>>   Now part of the large preallocated extent is shared.
>> - delalloc kicks in
>>   For the NOCOW range, as the preallocated extent is shared, we need
>>   to fall back to COW.
>>
>> [WORKAROUND]
>> The workaround is to ensure any buffered write in the related extents
>> (not the reflink source range) get flushed before reflink.
>>
>> However it's pretty expensive to do a comprehensive check.
>> In the reproducer, the reflink source is just a part of a larger
>> preallocated extent, we need to flush all buffered write of that extent
>> before reflink.
>> Such backward search can be complex and we may not get much benefit from
>> it.
>>
>> So this patch will just try to flush the whole inode before reflink.
> 
> Does that mean that if a large file is being written and deduped
> simultaneously, that the dedupes would now trigger flushes over the
> entire file?  That seems like it could be slow.

Yes, also my reason for RFC.

But it shouldn't be that heavy, as after the first dedupe/reflink, most
IO should be flushed back, later dedupe has much less work to do.


> 
> e.g. if the file is a big VM image file and it is used src and for dedupe
> (which is quite common in VM image files), we'd be hammering the disk
> with writes similar to hitting it with fsync() in a tight loop?

The original behavior also flush the target and source range, so we're
not completely creating some new overhead.

Thanks,
Qu

> 
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Reason for RFC:
>> Flushing an inode just because it's a reflink source is definitely
>> overkilling, but I don't have any better way to handle it.
>>
>> Any comment on this is welcomed.
>> ---
>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 7755b503b348..8caa0edb6fbf 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>  			return ret;
>>  	}
>>  
>> +	/*
>> +	 * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>> +	 *
>> +	 * Due to the limit of btrfs extent tree design, we can only have
>> +	 * extent level share view. Any part of an extent is shared then the
>> +	 * whole extent is shared and any write into that extent needs to fall
>> +	 * back to COW.
>> +	 *
>> +	 * NOCOW buffered write without data space reserved could to lead to
>> +	 * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>> +	 * at delalloc time (transaction abort).
>> +	 *
>> +	 * Here we take a shortcut by flush the whole inode. We could do better
>> +	 * by finding all extents in that range and flush the space referring
>> +	 * all those extents.
>> +	 * But that's too complex for such corner case.
>> +	 */
>> +	filemap_flush(src->i_mapping);
>> +	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>> +		     &BTRFS_I(src)->runtime_flags))
>> +		filemap_flush(src->i_mapping);
>> +
>>  	/*
>>  	 * Lock destination range to serialize with concurrent readpages() and
>>  	 * source range to serialize with relocation.
>> -- 
>> 2.21.0
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Zygo Blaxell]

[-- Attachment #1: Type: text/plain, Size: 6223 bytes --]

On Sat, May 04, 2019 at 08:32:11AM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/5/4 上午5:56, Zygo Blaxell wrote:
> > On Fri, May 03, 2019 at 09:08:52AM +0800, Qu Wenruo wrote:
> >> [BUG]
> >> The following command can lead to unexpected data COW:
> >>
> >>   #!/bin/bash
> >>
> >>   dev=/dev/test/test
> >>   mnt=/mnt/btrfs
> >>
> >>   mkfs.btrfs -f $dev -b 1G > /dev/null
> >>   mount $dev $mnt -o nospace_cache
> >>
> >>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
> >>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
> >>   umount $dev
> >>
> >> The result extent will be
> >>
> >> 	item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
> >> 		generation 6 type 2 (prealloc)
> >> 		prealloc data disk byte 13631488 nr 28672
> >> 	item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
> >> 		generation 6 type 1 (regular)
> >> 		extent data disk byte 13660160 nr 12288 <<< COW
> >> 	item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
> >> 		generation 6 type 2 (prealloc)
> >> 		prealloc data disk byte 13631488 nr 28672
> >>
> >> Currently we always reserve space even for NOCOW buffered write, thus
> >> under most case it shouldn't cause anything wrong even we fall back to
> >> COW.
> >>
> >> However when we're out of data space, we fall back to skip data space if
> >> we can do NOCOW write.
> >>
> >> If such behavior happens under that case, we could hit the following
> >> problems:
> >> - data space bytes_may_use underflow
> >>   This will cause kernel warning.
> >>
> >> - ENOSPC at delalloc time
> >>   This will lead to transaction abort and fs forced to RO.
> >>
> >> [CAUSE]
> >> This is due to the fact that btrfs can only do extent level share check.
> >>
> >> Btrfs can only tell if an extent is shared, no matter if only part of the
> >> extent is shared or not.
> >>
> >> So for above script we have:
> >> - fallocate
> >> - buffered write
> >>   If we don't have enough data space, we fall back to NOCOW check.
> >>   At this timming, the extent is not shared, we can skip data
> >>   reservation.
> >> - reflink
> >>   Now part of the large preallocated extent is shared.
> >> - delalloc kicks in
> >>   For the NOCOW range, as the preallocated extent is shared, we need
> >>   to fall back to COW.
> >>
> >> [WORKAROUND]
> >> The workaround is to ensure any buffered write in the related extents
> >> (not the reflink source range) get flushed before reflink.
> >>
> >> However it's pretty expensive to do a comprehensive check.
> >> In the reproducer, the reflink source is just a part of a larger
> >> preallocated extent, we need to flush all buffered write of that extent
> >> before reflink.
> >> Such backward search can be complex and we may not get much benefit from
> >> it.
> >>
> >> So this patch will just try to flush the whole inode before reflink.
> > 
> > Does that mean that if a large file is being written and deduped
> > simultaneously, that the dedupes would now trigger flushes over the
> > entire file?  That seems like it could be slow.
> 
> Yes, also my reason for RFC.
> 
> But it shouldn't be that heavy, as after the first dedupe/reflink, most
> IO should be flushed back, later dedupe has much less work to do.

Sure, but if writes are continuously happening (e.g. writes at offset
10GB, dedupe at 1GB), these will get flushed out on the next dedupe.
I'm thinking of scenarious where both writes and dedupes are happening
continuously, e.g. a host with multiple VM images running out of raw
image files that are deduped on the host filesystem.

I'm not sure what all the conditions for this flush are.  From the list
above, it sounds like this only occurs _after_ the filesystem has found
there is no free space.  If that's true, then the extra overhead is
negligible since it rarely happens (assuming that having no free space
is a rare condition for filesystems).


> > e.g. if the file is a big VM image file and it is used src and for dedupe
> > (which is quite common in VM image files), we'd be hammering the disk
> > with writes similar to hitting it with fsync() in a tight loop?
> 
> The original behavior also flush the target and source range, so we're
> not completely creating some new overhead.
> 
> Thanks,
> Qu
> 
> > 
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> Reason for RFC:
> >> Flushing an inode just because it's a reflink source is definitely
> >> overkilling, but I don't have any better way to handle it.
> >>
> >> Any comment on this is welcomed.
> >> ---
> >>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >> index 7755b503b348..8caa0edb6fbf 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> >>  			return ret;
> >>  	}
> >>  
> >> +	/*
> >> +	 * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> >> +	 *
> >> +	 * Due to the limit of btrfs extent tree design, we can only have
> >> +	 * extent level share view. Any part of an extent is shared then the
> >> +	 * whole extent is shared and any write into that extent needs to fall
> >> +	 * back to COW.
> >> +	 *
> >> +	 * NOCOW buffered write without data space reserved could to lead to
> >> +	 * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> >> +	 * at delalloc time (transaction abort).
> >> +	 *
> >> +	 * Here we take a shortcut by flush the whole inode. We could do better
> >> +	 * by finding all extents in that range and flush the space referring
> >> +	 * all those extents.
> >> +	 * But that's too complex for such corner case.
> >> +	 */
> >> +	filemap_flush(src->i_mapping);
> >> +	if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> >> +		     &BTRFS_I(src)->runtime_flags))
> >> +		filemap_flush(src->i_mapping);
> >> +
> >>  	/*
> >>  	 * Lock destination range to serialize with concurrent readpages() and
> >>  	 * source range to serialize with relocation.
> >> -- 
> >> 2.21.0
> >>
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Filipe Manana]

On Sun, May 5, 2019 at 4:33 PM Zygo Blaxell
<ce3g8jdj@umail.furryterror.org> wrote:
>
> On Sat, May 04, 2019 at 08:32:11AM +0800, Qu Wenruo wrote:
> >
> >
> > On 2019/5/4 上午5:56, Zygo Blaxell wrote:
> > > On Fri, May 03, 2019 at 09:08:52AM +0800, Qu Wenruo wrote:
> > >> [BUG]
> > >> The following command can lead to unexpected data COW:
> > >>
> > >>   #!/bin/bash
> > >>
> > >>   dev=/dev/test/test
> > >>   mnt=/mnt/btrfs
> > >>
> > >>   mkfs.btrfs -f $dev -b 1G > /dev/null
> > >>   mount $dev $mnt -o nospace_cache
> > >>
> > >>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
> > >>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
> > >>   umount $dev
> > >>
> > >> The result extent will be
> > >>
> > >>    item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
> > >>            generation 6 type 2 (prealloc)
> > >>            prealloc data disk byte 13631488 nr 28672
> > >>    item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
> > >>            generation 6 type 1 (regular)
> > >>            extent data disk byte 13660160 nr 12288 <<< COW
> > >>    item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
> > >>            generation 6 type 2 (prealloc)
> > >>            prealloc data disk byte 13631488 nr 28672
> > >>
> > >> Currently we always reserve space even for NOCOW buffered write, thus
> > >> under most case it shouldn't cause anything wrong even we fall back to
> > >> COW.
> > >>
> > >> However when we're out of data space, we fall back to skip data space if
> > >> we can do NOCOW write.
> > >>
> > >> If such behavior happens under that case, we could hit the following
> > >> problems:
> > >> - data space bytes_may_use underflow
> > >>   This will cause kernel warning.
> > >>
> > >> - ENOSPC at delalloc time
> > >>   This will lead to transaction abort and fs forced to RO.
> > >>
> > >> [CAUSE]
> > >> This is due to the fact that btrfs can only do extent level share check.
> > >>
> > >> Btrfs can only tell if an extent is shared, no matter if only part of the
> > >> extent is shared or not.
> > >>
> > >> So for above script we have:
> > >> - fallocate
> > >> - buffered write
> > >>   If we don't have enough data space, we fall back to NOCOW check.
> > >>   At this timming, the extent is not shared, we can skip data
> > >>   reservation.
> > >> - reflink
> > >>   Now part of the large preallocated extent is shared.
> > >> - delalloc kicks in
> > >>   For the NOCOW range, as the preallocated extent is shared, we need
> > >>   to fall back to COW.
> > >>
> > >> [WORKAROUND]
> > >> The workaround is to ensure any buffered write in the related extents
> > >> (not the reflink source range) get flushed before reflink.
> > >>
> > >> However it's pretty expensive to do a comprehensive check.
> > >> In the reproducer, the reflink source is just a part of a larger
> > >> preallocated extent, we need to flush all buffered write of that extent
> > >> before reflink.
> > >> Such backward search can be complex and we may not get much benefit from
> > >> it.
> > >>
> > >> So this patch will just try to flush the whole inode before reflink.
> > >
> > > Does that mean that if a large file is being written and deduped
> > > simultaneously, that the dedupes would now trigger flushes over the
> > > entire file?  That seems like it could be slow.
> >
> > Yes, also my reason for RFC.
> >
> > But it shouldn't be that heavy, as after the first dedupe/reflink, most
> > IO should be flushed back, later dedupe has much less work to do.
>
> Sure, but if writes are continuously happening (e.g. writes at offset
> 10GB, dedupe at 1GB), these will get flushed out on the next dedupe.
> I'm thinking of scenarious where both writes and dedupes are happening
> continuously, e.g. a host with multiple VM images running out of raw
> image files that are deduped on the host filesystem.
>
> I'm not sure what all the conditions for this flush are.  From the list
> above, it sounds like this only occurs _after_ the filesystem has found
> there is no free space.  If that's true, then the extra overhead is
> negligible since it rarely happens (assuming that having no free space
> is a rare condition for filesystems).

The problem is not that flush is done only when low on available space.
The flush would always happen on the entire source file before
reflinking, so that buffered writes that happened before the
clone/dedupe operation and "entered" nodatacow mode (because at the
time there was not enough available data space) will not fail when
their writeback starts - which would happen after the reflinking -
that's why the entire range is flushed.

Even if btrfs' reference counts are tracked per extent and not per
block, here we could maybe do something like check each reference,
extract the inode number, root number and offset. Then use that to
find the respective file extent items, and using those extract their
length and determine exactly which parts (blocks) of an extent are
shared. That would be a lot of work to do, and would always be racy
for checks for inodes that are not the inode we have locked for the
reflink operation. Very impractical.

So it's one more big inconvenience from the extent based back
references, other then the already known space wasting inconvenience
(even if only 1 block of an extent is really referenced, the rest of
the extent is unavailable for allocation, considered used space).



>
>
> > > e.g. if the file is a big VM image file and it is used src and for dedupe
> > > (which is quite common in VM image files), we'd be hammering the disk
> > > with writes similar to hitting it with fsync() in a tight loop?
> >
> > The original behavior also flush the target and source range, so we're
> > not completely creating some new overhead.
> >
> > Thanks,
> > Qu
> >
> > >
> > >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> > >> ---
> > >> Reason for RFC:
> > >> Flushing an inode just because it's a reflink source is definitely
> > >> overkilling, but I don't have any better way to handle it.
> > >>
> > >> Any comment on this is welcomed.
> > >> ---
> > >>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
> > >>  1 file changed, 22 insertions(+)
> > >>
> > >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > >> index 7755b503b348..8caa0edb6fbf 100644
> > >> --- a/fs/btrfs/ioctl.c
> > >> +++ b/fs/btrfs/ioctl.c
> > >> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> > >>                    return ret;
> > >>    }
> > >>
> > >> +  /*
> > >> +   * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
> > >> +   *
> > >> +   * Due to the limit of btrfs extent tree design, we can only have
> > >> +   * extent level share view. Any part of an extent is shared then the
> > >> +   * whole extent is shared and any write into that extent needs to fall
> > >> +   * back to COW.
> > >> +   *
> > >> +   * NOCOW buffered write without data space reserved could to lead to
> > >> +   * either data space bytes_may_use underflow (kernel warning) or ENOSPC
> > >> +   * at delalloc time (transaction abort).
> > >> +   *
> > >> +   * Here we take a shortcut by flush the whole inode. We could do better
> > >> +   * by finding all extents in that range and flush the space referring
> > >> +   * all those extents.
> > >> +   * But that's too complex for such corner case.
> > >> +   */
> > >> +  filemap_flush(src->i_mapping);
> > >> +  if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> > >> +               &BTRFS_I(src)->runtime_flags))
> > >> +          filemap_flush(src->i_mapping);
> > >> +
> > >>    /*
> > >>     * Lock destination range to serialize with concurrent readpages() and
> > >>     * source range to serialize with relocation.
> > >> --
> > >> 2.21.0
> > >>
> >
>
>
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5437 bytes --]



On 2019/5/6 上午12:24, Filipe Manana wrote:
[snip]
>>>
>>> Yes, also my reason for RFC.
>>>
>>> But it shouldn't be that heavy, as after the first dedupe/reflink, most
>>> IO should be flushed back, later dedupe has much less work to do.
>>
>> Sure, but if writes are continuously happening (e.g. writes at offset
>> 10GB, dedupe at 1GB), these will get flushed out on the next dedupe.
>> I'm thinking of scenarious where both writes and dedupes are happening
>> continuously, e.g. a host with multiple VM images running out of raw
>> image files that are deduped on the host filesystem.
>>
>> I'm not sure what all the conditions for this flush are.  From the list
>> above, it sounds like this only occurs _after_ the filesystem has found
>> there is no free space.  If that's true, then the extra overhead is
>> negligible since it rarely happens (assuming that having no free space
>> is a rare condition for filesystems).
> 
> The problem is not that flush is done only when low on available space.
> The flush would always happen on the entire source file before
> reflinking, so that buffered writes that happened before the
> clone/dedupe operation and "entered" nodatacow mode (because at the
> time there was not enough available data space) will not fail when
> their writeback starts - which would happen after the reflinking -
> that's why the entire range is flushed.
> 
> Even if btrfs' reference counts are tracked per extent and not per
> block, here we could maybe do something like check each reference,
> extract the inode number, root number and offset. Then use that to
> find the respective file extent items, and using those extract their
> length and determine exactly which parts (blocks) of an extent are
> shared. That would be a lot of work to do, and would always be racy
> for checks for inodes that are not the inode we have locked for the
> reflink operation. Very impractical.

To add my idea on better backref (block level), it's more impractical
than I thought.

From extent double/triple split, to how to handle old extents in old
snapshot, it's way too expensive from the developer's respect.

> 
> So it's one more big inconvenience from the extent based back
> references, other then the already known space wasting inconvenience
> (even if only 1 block of an extent is really referenced, the rest of
> the extent is unavailable for allocation, considered used space).

Currently this patch is only to be a workaround.

There is an idea of introducing new extent io tree bit, NODATACOW for
this case. Buffered write with NODATACOW will set the bit, and for the
specific problem described here, we only need to flush the range with
NODATACOW bit set.

And that bit can also be used to detect unexpected COW with no data
space reserved.

But that need extra work/testing (especially with my special sauce of
doing NODATACOW at buffered write time).
At least we have some idea on how to reduce the overhead.

Thanks,
Qu

> 
> 
> 
>>
>>
>>>> e.g. if the file is a big VM image file and it is used src and for dedupe
>>>> (which is quite common in VM image files), we'd be hammering the disk
>>>> with writes similar to hitting it with fsync() in a tight loop?
>>>
>>> The original behavior also flush the target and source range, so we're
>>> not completely creating some new overhead.
>>>
>>> Thanks,
>>> Qu
>>>
>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>> Reason for RFC:
>>>>> Flushing an inode just because it's a reflink source is definitely
>>>>> overkilling, but I don't have any better way to handle it.
>>>>>
>>>>> Any comment on this is welcomed.
>>>>> ---
>>>>>  fs/btrfs/ioctl.c | 22 ++++++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>> index 7755b503b348..8caa0edb6fbf 100644
>>>>> --- a/fs/btrfs/ioctl.c
>>>>> +++ b/fs/btrfs/ioctl.c
>>>>> @@ -3930,6 +3930,28 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>>>>>                    return ret;
>>>>>    }
>>>>>
>>>>> +  /*
>>>>> +   * Workaround to make sure NOCOW buffered write reach disk as NOCOW.
>>>>> +   *
>>>>> +   * Due to the limit of btrfs extent tree design, we can only have
>>>>> +   * extent level share view. Any part of an extent is shared then the
>>>>> +   * whole extent is shared and any write into that extent needs to fall
>>>>> +   * back to COW.
>>>>> +   *
>>>>> +   * NOCOW buffered write without data space reserved could to lead to
>>>>> +   * either data space bytes_may_use underflow (kernel warning) or ENOSPC
>>>>> +   * at delalloc time (transaction abort).
>>>>> +   *
>>>>> +   * Here we take a shortcut by flush the whole inode. We could do better
>>>>> +   * by finding all extents in that range and flush the space referring
>>>>> +   * all those extents.
>>>>> +   * But that's too complex for such corner case.
>>>>> +   */
>>>>> +  filemap_flush(src->i_mapping);
>>>>> +  if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
>>>>> +               &BTRFS_I(src)->runtime_flags))
>>>>> +          filemap_flush(src->i_mapping);
>>>>> +
>>>>>    /*
>>>>>     * Lock destination range to serialize with concurrent readpages() and
>>>>>     * source range to serialize with relocation.
>>>>> --
>>>>> 2.21.0
>>>>>
>>>
>>
>>
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC] btrfs: reflink: Flush before reflink any extent to prevent NOCOW write falling back to CoW without data reservation

[Josef Bacik]

On Fri, May 03, 2019 at 09:08:52AM +0800, Qu Wenruo wrote:
> [BUG]
> The following command can lead to unexpected data COW:
> 
>   #!/bin/bash
> 
>   dev=/dev/test/test
>   mnt=/mnt/btrfs
> 
>   mkfs.btrfs -f $dev -b 1G > /dev/null
>   mount $dev $mnt -o nospace_cache
> 
>   xfs_io -f -c "falloc 8k 24k" -c "pwrite 12k 8k" $mnt/file1
>   xfs_io -c "reflink $mnt/file1 8k 0 4k" $mnt/file1
>   umount $dev
> 
> The result extent will be
> 
> 	item 7 key (257 EXTENT_DATA 4096) itemoff 15760 itemsize 53
> 		generation 6 type 2 (prealloc)
> 		prealloc data disk byte 13631488 nr 28672
> 	item 8 key (257 EXTENT_DATA 12288) itemoff 15707 itemsize 53
> 		generation 6 type 1 (regular)
> 		extent data disk byte 13660160 nr 12288 <<< COW
> 	item 9 key (257 EXTENT_DATA 24576) itemoff 15654 itemsize 53
> 		generation 6 type 2 (prealloc)
> 		prealloc data disk byte 13631488 nr 28672
> 
> Currently we always reserve space even for NOCOW buffered write, thus
> under most case it shouldn't cause anything wrong even we fall back to
> COW.
> 
> However when we're out of data space, we fall back to skip data space if
> we can do NOCOW write.
> 
> If such behavior happens under that case, we could hit the following
> problems:
> - data space bytes_may_use underflow
>   This will cause kernel warning.
> 

This can be fixed, I laid out a few ways it could be fixed.

> - ENOSPC at delalloc time
>   This will lead to transaction abort and fs forced to RO.
> 

How?  The metadata and data reservations are separate.  If we can't make the
metadata reservation we fail out, the only thing we allow is skipping the data
reservation.  So if we fall back to cow_file_range() at run_delalloc_nocow()
time all we'll do is get an ENOSPC outside of a transaction, so we can just
mark the inode as having failed its writeout with ENOSPC so fsync() returns the
appropriate error and carry on.  We shouldn't be aborting a transaction here at
all.  Thanks,

Josef