[BUG?] Defrag on compressed FS do massive data rewrites

[BUG?] Defrag on compressed FS do massive data rewrites

[Timofey Titovets]

Hi, i see massive data rewrites of defragmented files when work with
btrfs fi def <args>.
Before, i just thought it's a design problem - i.e. defrag always
rewrite data to new place.

At now, i read the code and see 2 bad cases:
1. With -c<compression_algo> all extents of data will be rewriten, always.
2. btrfs use "bad" default target extent size, i.e. kernel by default
try get 256KiB extent, btrfs-progres use 32MiB as a threshold.
 Both of them make ioctl code should_defrag_range() think, that extent
are "too" fragmented, and rewrite all compressed extents.

Does that behavior expected?

i.e. only way that i can safely use on my data are:
btrfs fi def -vr -t 128KiB <path>
That will defrag all fragmented compressed extents.

"Hacky" solution that i see for now, is a create copy of inode_need_compress()
for defrag ioctl, and if file must be compressed, force use of 128KiB
as target extent.
or at least document that not obvious behaviour.

Thanks!

-- 
Have a nice day,
Timofey.

Re: [BUG?] Defrag on compressed FS do massive data rewrites

[Timofey Titovets]

2017-12-14 1:09 GMT+03:00 Timofey Titovets <nefelim4ag@gmail.com>:
> Hi, i see massive data rewrites of defragmented files when work with
> btrfs fi def <args>.
> Before, i just thought it's a design problem - i.e. defrag always
> rewrite data to new place.
>
> At now, i read the code and see 2 bad cases:
> 1. With -c<compression_algo> all extents of data will be rewriten, always.
> 2. btrfs use "bad" default target extent size, i.e. kernel by default
> try get 256KiB extent, btrfs-progres use 32MiB as a threshold.
>  Both of them make ioctl code should_defrag_range() think, that extent
> are "too" fragmented, and rewrite all compressed extents.
>
> Does that behavior expected?
>
> i.e. only way that i can safely use on my data are:
> btrfs fi def -vr -t 128KiB <path>
> That will defrag all fragmented compressed extents.
>
> "Hacky" solution that i see for now, is a create copy of inode_need_compress()
> for defrag ioctl, and if file must be compressed, force use of 128KiB
> as target extent.
> or at least document that not obvious behaviour.
>
> Thanks!
>
> --
> Have a nice day,
> Timofey.

Also, same problem exist for autodefrag case
i.e.:
write 4KiB at start of compressed file
autodefrag code add that file to autodefrag queue, call
btrfs_defrag_file, set range from start to u64-1.
That will trigger to full file rewrite, as all extents are smaller then 256KiB.

(if i understood all correctly).

Thanks.


-- 
Have a nice day,
Timofey.

Re: [BUG?] Defrag on compressed FS do massive data rewrites

[Duncan]

Timofey Titovets posted on Thu, 14 Dec 2017 01:09:52 +0300 as excerpted:

> Hi, i see massive data rewrites of defragmented files when work with
> btrfs fi def <args>.
> Before, i just thought it's a design problem - i.e. defrag always
> rewrite data to new place.
> 
> At now, i read the code and see 2 bad cases:
> 1. With -c<compression_algo> all extents of data will be rewriten,
> always.

Based on previous threads this is indeed expected behavior -- it's the 
recommendation for those who want to recompress to a different target 
compression level, so rewriting *everything* the command line points at 
is required.

And FWIW that's exactly how I'd interpret the manpage's description of 
the -c option as well: "Compress file contents while defragging", with 
the example using defrag -c later in the manpage explaining it as "force 
file compression."

So I'd call this behavior documented.

> 2. btrfs use "bad" default target extent size, i.e. kernel by default
> try get 256KiB extent, btrfs-progres use 32MiB as a threshold.
>  Both of them make ioctl code should_defrag_range() think, that extent
> are "too" fragmented, and rewrite all compressed extents.

I've not seen this mentioned previously, and it certainly does surprise 
me.  I suspect in this case it's accidental, simply the result of btrfs 
choosing a 128 KiB compression block, while whoever did the defrag code 
wasn't thinking about the effects of a 256 KiB or 32 MiB target size on 
compressed extents, which always show up as 128 KiB due to that being the 
compression block size, even if they're adjacent and thus not /really/ 
fragmented.

What /really/ surprises me, tho, is that yes, it's known that filefrag 
reports the wrong thing for compressed files, but btrfs is the one doing 
it that way in the first place, and /it/ can't figure out that they're 
not actually fragmented, even for calculations like this where getting 
the actual fragmentation is the entire point?  That's crazy!

> Does that behavior expected?
> 
> i.e. only way that i can safely use on my data are:
> btrfs fi def -vr -t 128KiB <path>
> That will defrag all fragmented compressed extents.

Given the above that appears to be accurate.  If you don't want the 
compressed extents rewritten, don't supply the -c/compress option and 
rely on the compress mount option to do the default compression it would 
do on any other write.  And set -t 128K (according to the manpage, it's 
just K, not KiB, don't know if it actually takes KiB as I'm an admin 
reading manpage, not a coder reading code) so it considers full 
compression blocks defragged.

Tho that assumes you're correct and the code doesn't make allowances for 
compression-block size elsewhere, but I have no reason to assume 
otherwise unless a dev points it out.

> "Hacky" solution that i see for now, is a create copy of
> inode_need_compress()
> for defrag ioctl, and if file must be compressed, force use of 128KiB as
> target extent.
> or at least document that not obvious behaviour.

#2 really does need documented or changed, yes.  It's certainly 
surprising, here.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [BUG?] Defrag on compressed FS do massive data rewrites

[Duncan]

Timofey Titovets posted on Thu, 14 Dec 2017 02:05:35 +0300 as excerpted:

> Also, same problem exist for autodefrag case i.e.:
> write 4KiB at start of compressed file autodefrag code add that file to
> autodefrag queue, call btrfs_defrag_file, set range from start to u64-1.
> That will trigger to full file rewrite, as all extents are smaller then
> 256KiB.
> 
> (if i understood all correctly).

If so, it's rather ironic, because that's how I believed autodefrag to 
work, whole-file, for quite some time.  Then I learned otherwise, but I 
always enable both autodefrag and compress=lzo on all my btrfs, so it 
looks like at least for my use-case, I was correct with the whole-file 
assumption after all.  (Well, at least for files that are actually 
compressed, I don't run compress-force=lzo, just compress=lzo, so a 
reasonable number of files aren't actually compressed anyway, and they'd 
do the partial-file rewrite I had learned to be normal.)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [BUG?] Defrag on compressed FS do massive data rewrites

[Timofey Titovets]

2017-12-14 8:58 GMT+03:00 Duncan <1i5t5.duncan@cox.net>:
> Timofey Titovets posted on Thu, 14 Dec 2017 02:05:35 +0300 as excerpted:
>
>> Also, same problem exist for autodefrag case i.e.:
>> write 4KiB at start of compressed file autodefrag code add that file to
>> autodefrag queue, call btrfs_defrag_file, set range from start to u64-1.
>> That will trigger to full file rewrite, as all extents are smaller then
>> 256KiB.
>>
>> (if i understood all correctly).
>
> If so, it's rather ironic, because that's how I believed autodefrag to
> work, whole-file, for quite some time.  Then I learned otherwise, but I
> always enable both autodefrag and compress=lzo on all my btrfs, so it
> looks like at least for my use-case, I was correct with the whole-file
> assumption after all.  (Well, at least for files that are actually
> compressed, I don't run compress-force=lzo, just compress=lzo, so a
> reasonable number of files aren't actually compressed anyway, and they'd
> do the partial-file rewrite I had learned to be normal.)
>
> --
> Duncan - List replies preferred.   No HTML msgs.
> "Every nonfree program has a lord, a master --
> and if you use the program, he is your master."  Richard Stallman
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

btrfs fi def can easy avoid that problem if properly documented,
but i think that fix must be in place, because "full rewrite" by
autodefrag are unacceptable behaviour.

How i see, "How it works":
Both, defrag ioctl and autodefrag - call btrfs_defrag_file() (ioctl.c).
btrfs_defrag_file() set extent_thresh to args, if args not provided
(autodefrag not initialize range->extent_thresh);
use default:
if (extent_thresh == 0)
     extent_thresh = SZ_256K;

Later btrfs_defrag_file() try defrag file from start by "index" (page
number from start, file virtually splitted to page sized blocks).
Than it call should_defrag_range(), if need make i+1 or skip range by
info from should_defrag_range().

should_defrag_range() get extent for specified start offset:
em = defrag_lookup_extent(inode, start);

Later (em->len >= thresh || (!next_mergeable && !prev_mergeable)) will
fail condition because len (<128KiB) < thresh (256KiB or 32MiB usual).
So extent will be rewritten.

struct extent_map{}; have two potential useful info:

...
u64 len;
...

unsigned int compress_type;
...

As i see by code len store "real" length, so
in theory that just need to add additional check later like:
if (em->len = BTRFS_MAX_UNCOMPRESSED && em->compress_type > 0)
    ret = 0;

That must fix problem by "true" insight check for compressed extents.

Thanks.

P.S.
(May be someone from more experienced devs can comment that?)

-- 
Have a nice day,
Timofey.

Re: [BUG?] Defrag on compressed FS do massive data rewrites

[Timofey Titovets]

I send:
[PATCH] Btrfs: make should_defrag_range() understood compressed extents

If you want you can test that, it fix btrfs fi def and autodefrag
behavior with compressed data.
Also it understood if user try recompress data with old/new compression algo.

2017-12-14 14:27 GMT+03:00 Timofey Titovets <nefelim4ag@gmail.com>:
> 2017-12-14 8:58 GMT+03:00 Duncan <1i5t5.duncan@cox.net>:
>> Timofey Titovets posted on Thu, 14 Dec 2017 02:05:35 +0300 as excerpted:
>>
>>> Also, same problem exist for autodefrag case i.e.:
>>> write 4KiB at start of compressed file autodefrag code add that file to
>>> autodefrag queue, call btrfs_defrag_file, set range from start to u64-1.
>>> That will trigger to full file rewrite, as all extents are smaller then
>>> 256KiB.
>>>
>>> (if i understood all correctly).
>>
>> If so, it's rather ironic, because that's how I believed autodefrag to
>> work, whole-file, for quite some time.  Then I learned otherwise, but I
>> always enable both autodefrag and compress=lzo on all my btrfs, so it
>> looks like at least for my use-case, I was correct with the whole-file
>> assumption after all.  (Well, at least for files that are actually
>> compressed, I don't run compress-force=lzo, just compress=lzo, so a
>> reasonable number of files aren't actually compressed anyway, and they'd
>> do the partial-file rewrite I had learned to be normal.)
>>
>> --
>> Duncan - List replies preferred.   No HTML msgs.
>> "Every nonfree program has a lord, a master --
>> and if you use the program, he is your master."  Richard Stallman
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> btrfs fi def can easy avoid that problem if properly documented,
> but i think that fix must be in place, because "full rewrite" by
> autodefrag are unacceptable behaviour.
>
> How i see, "How it works":
> Both, defrag ioctl and autodefrag - call btrfs_defrag_file() (ioctl.c).
> btrfs_defrag_file() set extent_thresh to args, if args not provided
> (autodefrag not initialize range->extent_thresh);
> use default:
> if (extent_thresh == 0)
>      extent_thresh = SZ_256K;
>
> Later btrfs_defrag_file() try defrag file from start by "index" (page
> number from start, file virtually splitted to page sized blocks).
> Than it call should_defrag_range(), if need make i+1 or skip range by
> info from should_defrag_range().
>
> should_defrag_range() get extent for specified start offset:
> em = defrag_lookup_extent(inode, start);
>
> Later (em->len >= thresh || (!next_mergeable && !prev_mergeable)) will
> fail condition because len (<128KiB) < thresh (256KiB or 32MiB usual).
> So extent will be rewritten.
>
> struct extent_map{}; have two potential useful info:
>
> ...
> u64 len;
> ...
>
> unsigned int compress_type;
> ...
>
> As i see by code len store "real" length, so
> in theory that just need to add additional check later like:
> if (em->len = BTRFS_MAX_UNCOMPRESSED && em->compress_type > 0)
>     ret = 0;
>
> That must fix problem by "true" insight check for compressed extents.
>
> Thanks.
>
> P.S.
> (May be someone from more experienced devs can comment that?)
>
> --
> Have a nice day,
> Timofey.



-- 
Have a nice day,
Timofey.