diff mbox series

[v3,13/13] bcachefs: fiemap: emit new COMPRESSED state

Message ID 943938ff75580b210eebf6c885659dd95f029486.1712126039.git.sweettea-kernel@dorminy.me (mailing list archive)
State New, archived
Headers show
Series fiemap extension for more physical information | expand

Commit Message

Sweet Tea Dorminy April 3, 2024, 7:22 a.m. UTC
Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/bcachefs/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Dilger April 5, 2024, 7:17 p.m. UTC | #1
On Apr 3, 2024, at 1:22 AM, Sweet Tea Dorminy <sweettea-kernel@dorminy.me> wrote:
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> fs/bcachefs/fs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> index d2793bae842d..54f613f977b4 100644
> --- a/fs/bcachefs/fs.c
> +++ b/fs/bcachefs/fs.c
> @@ -921,7 +921,7 @@ static int bch2_fill_extent(struct bch_fs *c,
> 				flags2 |= FIEMAP_EXTENT_UNWRITTEN;
> 
> 			if (p.crc.compression_type) {
> -				flags2 |= FIEMAP_EXTENT_ENCODED;
> +				flags2 |= FIEMAP_EXTENT_DATA_COMPRESSED;

(defect) This should *also* set FIEMAP_EXTENT_ENCODED in this case,
along with FIEMAP_EXTENT_DATA_COMPRESSED.  Both for compatibility with
older code that doesn't understand FIEMAP_EXTENT_DATA_COMPRESSED, and
because the data still cannot be read directly from the volume when it
is not mounted.

Probably Kent should chime in here with what needs to be done to set
the phys_len properly for bcachefs, or leave this patch out of your
series and let him submit it directly.  With proposed wrapper in the
first patch of the series there isn't a hard requirement to change
all of the filesystems in one shot.

Cheers, Andreas
Andreas Dilger April 5, 2024, 7:34 p.m. UTC | #2
> On Apr 5, 2024, at 1:17 PM, Andreas Dilger <adilger@dilger.ca> wrote:
> 
> On Apr 3, 2024, at 1:22 AM, Sweet Tea Dorminy <sweettea-kernel@dorminy.me> wrote:
>> 
>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
>> ---
>> fs/bcachefs/fs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
>> index d2793bae842d..54f613f977b4 100644
>> --- a/fs/bcachefs/fs.c
>> +++ b/fs/bcachefs/fs.c
>> @@ -921,7 +921,7 @@ static int bch2_fill_extent(struct bch_fs *c,
>> 				flags2 |= FIEMAP_EXTENT_UNWRITTEN;
>> 
>> 			if (p.crc.compression_type) {
>> -				flags2 |= FIEMAP_EXTENT_ENCODED;
>> +				flags2 |= FIEMAP_EXTENT_DATA_COMPRESSED;
> 
> (defect) This should *also* set FIEMAP_EXTENT_ENCODED in this case,
> along with FIEMAP_EXTENT_DATA_COMPRESSED.  Both for compatibility with
> older code that doesn't understand FIEMAP_EXTENT_DATA_COMPRESSED, and
> because the data still cannot be read directly from the volume when it
> is not mounted.
> 
> Probably Kent should chime in here with what needs to be done to set
> the phys_len properly for bcachefs, or leave this patch out of your
> series and let him submit it directly.  With proposed wrapper in the
> first patch of the series there isn't a hard requirement to change
> all of the filesystems in one shot.

Ah, I missed the 11/13 patch that is handling up most of the bcachefs
phys_len changes.  I think this should be folded into that patch so
it is clear to the callers that the data is compressed when they see
fe_physical_length is not the same as fe_logical_length.

Cheers, Andreas
Kent Overstreet April 6, 2024, 5:20 a.m. UTC | #3
On Fri, Apr 05, 2024 at 01:17:45PM -0600, Andreas Dilger wrote:
> On Apr 3, 2024, at 1:22 AM, Sweet Tea Dorminy <sweettea-kernel@dorminy.me> wrote:
> > 
> > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> > ---
> > fs/bcachefs/fs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > index d2793bae842d..54f613f977b4 100644
> > --- a/fs/bcachefs/fs.c
> > +++ b/fs/bcachefs/fs.c
> > @@ -921,7 +921,7 @@ static int bch2_fill_extent(struct bch_fs *c,
> > 				flags2 |= FIEMAP_EXTENT_UNWRITTEN;
> > 
> > 			if (p.crc.compression_type) {
> > -				flags2 |= FIEMAP_EXTENT_ENCODED;
> > +				flags2 |= FIEMAP_EXTENT_DATA_COMPRESSED;
> 
> (defect) This should *also* set FIEMAP_EXTENT_ENCODED in this case,
> along with FIEMAP_EXTENT_DATA_COMPRESSED.  Both for compatibility with
> older code that doesn't understand FIEMAP_EXTENT_DATA_COMPRESSED, and
> because the data still cannot be read directly from the volume when it
> is not mounted.
> 
> Probably Kent should chime in here with what needs to be done to set
> the phys_len properly for bcachefs, or leave this patch out of your
> series and let him submit it directly.  With proposed wrapper in the
> first patch of the series there isn't a hard requirement to change
> all of the filesystems in one shot.

You get phys len from crc.compressed_size - that's always guaranteed,
even if it's not compressed
diff mbox series

Patch

diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
index d2793bae842d..54f613f977b4 100644
--- a/fs/bcachefs/fs.c
+++ b/fs/bcachefs/fs.c
@@ -921,7 +921,7 @@  static int bch2_fill_extent(struct bch_fs *c,
 				flags2 |= FIEMAP_EXTENT_UNWRITTEN;
 
 			if (p.crc.compression_type) {
-				flags2 |= FIEMAP_EXTENT_ENCODED;
+				flags2 |= FIEMAP_EXTENT_DATA_COMPRESSED;
 				phys_len = p.crc.compressed_size << 9;
 			} else
 				offset += p.crc.offset;