[PATCH][Ext4Pkg] unwritten extent suuport

[PATCH][Ext4Pkg] unwritten extent suuport

[atmgnd]

From: "Qi Zhou" <atmgnd@outlook.com>
Subject: [PATCH] unwritten extent suuport

the real lenght of uninitialized/unwritten extent should be (ee_len - (1UL << 15)), and
all related block should been read as zeros. see:
https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156

Signed-off-by: Qi Zhou <atmgnd@outlook.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
 Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
 Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
index 070eb5a..7ca8eee 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
@@ -402,6 +402,11 @@ typedef struct {
 
 #define EXT4_MIN_DIR_ENTRY_LEN  8
 
+#define EXTENT_INIT_MAX_LEN (1UL << 15)
+
+#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x : (x - EXTENT_INIT_MAX_LEN)))
+#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
+
 // This on-disk structure is present at the bottom of the extent tree
 typedef struct {
   // First logical block
diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
index 5fa2fe0..21af573 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
@@ -332,7 +332,7 @@ Ext4GetExtent (
     return EFI_NO_MAPPING;
   }
 
-  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block + Ext->ee_len > LogicalBlock)) {
+  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block + EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
     // This extent does not cover the block
     if (Buffer != NULL) {
       FreePool (Buffer);
@@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
   Extent = UserStruct;
   Block  = (UINT32)(UINTN)StandaloneKey;
 
-  if (Block >= Extent->ee_block && Block < Extent->ee_block + Extent->ee_len) {
+  if (Block >= Extent->ee_block && Block < Extent->ee_block + EXTENT_REAL_LEN(Extent->ee_len)) {
     return 0;
   }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
index 63cecec..d691ec7 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
@@ -151,6 +151,11 @@ Ext4Read (
       // Potential improvement: In the future, we could get the hole's tota
       // size and memset all that
       SetMem (Buffer, WasRead, 0);
+    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
+      HoleOff = CurrentSeek - (UINT64)Extent.ee_block * Partition->BlockSize;
+      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) * Partition->BlockSize - HoleOff;
+      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
+      SetMem (Buffer, WasRead, 0);
     } else {
       ExtentStartBytes = MultU64x32 (
                            LShiftU64 (Extent.ee_start_hi, 32) |
-- 
2.17.1

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[Pedro Falcato]

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

Hi,

The patch looks OK despite the typos and lack of proper formatting on the
commit message.

But honestly, I don't know if this patch is even mergeable considering you
looked at the Linux kernel's source code for this. The patch was already
trivial enough
if you looked at the documentation and the FreeBSD driver (as I had done in
the past but never got to fixing this considering I don't even know if
unwritten extents can appear in the wild).

I *cannot* stress this enough: Ext4Pkg is a clean room implementation of
ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
compatible with GPLv2) and cannot have random Linux kernel
bits, or any other incompatibly-licensed project's bits for that matter.

Best regards,
Pedro

On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:

> From: "Qi Zhou" <atmgnd@outlook.com>
> Subject: [PATCH] unwritten extent suuport
>
> the real lenght of uninitialized/unwritten extent should be (ee_len - (1UL
> << 15)), and
> all related block should been read as zeros. see:
>
> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
>
> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
> ---
>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
>  3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> index 070eb5a..7ca8eee 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> @@ -402,6 +402,11 @@ typedef struct {
>
>  #define EXT4_MIN_DIR_ENTRY_LEN  8
>
> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
> +
> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x : (x -
> EXTENT_INIT_MAX_LEN)))
> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
> +
>  // This on-disk structure is present at the bottom of the extent tree
>  typedef struct {
>    // First logical block
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> index 5fa2fe0..21af573 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> @@ -332,7 +332,7 @@ Ext4GetExtent (
>      return EFI_NO_MAPPING;
>    }
>
> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block + Ext->ee_len >
> LogicalBlock)) {
> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
>      // This extent does not cover the block
>      if (Buffer != NULL) {
>        FreePool (Buffer);
> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
>    Extent = UserStruct;
>    Block  = (UINT32)(UINTN)StandaloneKey;
>
> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> Extent->ee_len) {
> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> EXTENT_REAL_LEN(Extent->ee_len)) {
>      return 0;
>    }
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> index 63cecec..d691ec7 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> @@ -151,6 +151,11 @@ Ext4Read (
>        // Potential improvement: In the future, we could get the hole's
> tota
>        // size and memset all that
>        SetMem (Buffer, WasRead, 0);
> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block *
> Partition->BlockSize;
> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) * Partition->BlockSize -
> HoleOff;
> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
> +      SetMem (Buffer, WasRead, 0);
>      } else {
>        ExtentStartBytes = MultU64x32 (
>                             LShiftU64 (Extent.ee_start_hi, 32) |
> --
> 2.17.1
>
>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 5222 bytes --]

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[qi zhou]

1. I am not familiar with freebsd, and don know if freebsd get the same issue,
But I do found the freebsd has some code snippets related to unwritten extent,
see: https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
Is Ext4 is freebsd's default/major file system ?

2. I did't look at linux kernel(ext4) berfor send this patch, I cant
found any offcial document, so I refer to linux source as a standand
when send this patch

3. Yes, unwritten extents are wild used, usally when a file cotains many
zeros, or mark file holes(fallocate, qemu-img...)
You can generate a file contains a lot of unwritten extents by qemu, for
example:
qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
# win10.img's size: 10G
But for files do not have any continuous zeros, like compressed files,
then there will be no any unwritten extents
unwritten extents are usally seen in very large files

4. You can fix the typos, My English is not so good

On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com> wrote:

> Hi,
>  
> The patch looks OK despite the typos and lack of proper formatting on
> the commit message.
>  
> But honestly, I don't know if this patch is even mergeable considering
> you looked at the Linux kernel's source code for this. The patch was
> already trivial enough
> if you looked at the documentation and the FreeBSD driver (as I had
> done in the past but never got to fixing this considering I don't even
> know if unwritten extents can appear in the wild).
>  
> I *cannot* stress this enough: Ext4Pkg is a clean room implementation
> of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
> compatible with GPLv2) and cannot have random Linux kernel
> bits, or any other incompatibly-licensed project's bits for that matter.
>  
> Best regards,
> Pedro
>  
>  
>> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
>>  
>>> From: "Qi Zhou" <atmgnd@outlook.com>
>>> Subject: [PATCH] unwritten extent suuport
>>>  
>>> the real lenght of uninitialized/unwritten extent should be (ee_len
>>> - (1UL << 15)), and
>>> all related block should been read as zeros. see:
>>> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
>>>  
>>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
>>> ---
>>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
>>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
>>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>  
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> index 070eb5a..7ca8eee 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> @@ -402,6 +402,11 @@ typedef struct {
>>>  
>>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
>>>  
>>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
>>> +
>>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
>>> (x - EXTENT_INIT_MAX_LEN)))
>>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
>>> +
>>>  // This on-disk structure is present at the bottom of the extent tree
>>>  typedef struct {
>>>    // First logical block
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> index 5fa2fe0..21af573 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> @@ -332,7 +332,7 @@ Ext4GetExtent (
>>>      return EFI_NO_MAPPING;
>>>    }
>>>  
>>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> Ext->ee_len > LogicalBlock)) {
>>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
>>>      // This extent does not cover the block
>>>      if (Buffer != NULL) {
>>>        FreePool (Buffer);
>>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
>>>    Extent = UserStruct;
>>>    Block  = (UINT32)(UINTN)StandaloneKey;
>>>  
>>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> Extent->ee_len) {
>>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> EXTENT_REAL_LEN(Extent->ee_len)) {
>>>      return 0;
>>>    }
>>>  
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> index 63cecec..d691ec7 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> @@ -151,6 +151,11 @@ Ext4Read (
>>>        // Potential improvement: In the future, we could get the
>>> hole's tota
>>>        // size and memset all that
>>>        SetMem (Buffer, WasRead, 0);
>>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
>>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block * Partition->BlockSize;
>>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
>>> Partition->BlockSize - HoleOff;
>>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
>>> +      SetMem (Buffer, WasRead, 0);
>>>      } else {
>>>        ExtentStartBytes = MultU64x32 (
>>>                             LShiftU64 (Extent.ee_start_hi, 32) |
>>> --
>>> 2.17.1
>>>  
>  
>  
> --
>  
> Pedro Falcato

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[qi zhou]

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

But I do googled

________________________________
From: QiZhou <atmgnd@outlook.com>
Sent: Wednesday, October 27, 2021 23:44
To: pedro.falcato@gmail.com <pedro.falcato@gmail.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>
Subject: Re: [PATCH][Ext4Pkg] unwritten extent suuport

1. I am not familiar with freebsd, and don know if freebsd get the same issue,
But I do found the freebsd has some code snippets related to unwritten extent,
see: https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
Is Ext4 is freebsd's default/major file system ?

2. I did't look at linux kernel(ext4) berfor send this patch, I cant
found any offcial document, so I refer to linux source as a standand
when send this patch

3. Yes, unwritten extents are wild used, usally when a file cotains many
zeros, or mark file holes(fallocate, qemu-img...)
You can generate a file contains a lot of unwritten extents by qemu, for
example:
qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
# win10.img's size: 10G
But for files do not have any continuous zeros, like compressed files,
then there will be no any unwritten extents
unwritten extents are usally seen in very large files

4. You can fix the typos, My English is not so good

On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com> wrote:

> Hi,
>
> The patch looks OK despite the typos and lack of proper formatting on
> the commit message.
>
> But honestly, I don't know if this patch is even mergeable considering
> you looked at the Linux kernel's source code for this. The patch was
> already trivial enough
> if you looked at the documentation and the FreeBSD driver (as I had
> done in the past but never got to fixing this considering I don't even
> know if unwritten extents can appear in the wild).
>
> I *cannot* stress this enough: Ext4Pkg is a clean room implementation
> of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
> compatible with GPLv2) and cannot have random Linux kernel
> bits, or any other incompatibly-licensed project's bits for that matter.
>
> Best regards,
> Pedro
>
>
>> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
>>
>>> From: "Qi Zhou" <atmgnd@outlook.com>
>>> Subject: [PATCH] unwritten extent suuport
>>>
>>> the real lenght of uninitialized/unwritten extent should be (ee_len
>>> - (1UL << 15)), and
>>> all related block should been read as zeros. see:
>>> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
>>>
>>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
>>> ---
>>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
>>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
>>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> index 070eb5a..7ca8eee 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> @@ -402,6 +402,11 @@ typedef struct {
>>>
>>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
>>>
>>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
>>> +
>>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
>>> (x - EXTENT_INIT_MAX_LEN)))
>>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
>>> +
>>>  // This on-disk structure is present at the bottom of the extent tree
>>>  typedef struct {
>>>    // First logical block
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> index 5fa2fe0..21af573 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> @@ -332,7 +332,7 @@ Ext4GetExtent (
>>>      return EFI_NO_MAPPING;
>>>    }
>>>
>>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> Ext->ee_len > LogicalBlock)) {
>>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
>>>      // This extent does not cover the block
>>>      if (Buffer != NULL) {
>>>        FreePool (Buffer);
>>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
>>>    Extent = UserStruct;
>>>    Block  = (UINT32)(UINTN)StandaloneKey;
>>>
>>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> Extent->ee_len) {
>>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> EXTENT_REAL_LEN(Extent->ee_len)) {
>>>      return 0;
>>>    }
>>>
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> index 63cecec..d691ec7 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> @@ -151,6 +151,11 @@ Ext4Read (
>>>        // Potential improvement: In the future, we could get the
>>> hole's tota
>>>        // size and memset all that
>>>        SetMem (Buffer, WasRead, 0);
>>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
>>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block * Partition->BlockSize;
>>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
>>> Partition->BlockSize - HoleOff;
>>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
>>> +      SetMem (Buffer, WasRead, 0);
>>>      } else {
>>>        ExtentStartBytes = MultU64x32 (
>>>                             LShiftU64 (Extent.ee_start_hi, 32) |
>>> --
>>> 2.17.1
>>>
>
>
> --
>
> Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 8931 bytes --]

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[Pedro Falcato]

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

Hi Qi,

If you didn't use the Linux kernel (nor the documentation) as a reference,
can you please tell me what you've used? I'm asking because there's at
least a line that's suspiciously similar to Linux's code:

#define EXTENT_INIT_MAX_LEN (1UL << 15)

the UL looks redundant to me, since there's no need for it.

Also, I prefer that you fix the typos yourself and format the patch
correctly, including the code.


On Wed, Oct 27, 2021 at 4:45 PM QiZhou <atmgnd@outlook.com> wrote:

> 1. I am not familiar with freebsd, and don know if freebsd get the same
> issue,
> But I do found the freebsd has some code snippets related to unwritten
> extent,
> see:
> https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
>
> https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
> Is Ext4 is freebsd's default/major file system ?
>
> 2. I did't look at linux kernel(ext4) berfor send this patch, I cant
> found any offcial document, so I refer to linux source as a standand
> when send this patch
>
> 3. Yes, unwritten extents are wild used, usally when a file cotains many
> zeros, or mark file holes(fallocate, qemu-img...)
> You can generate a file contains a lot of unwritten extents by qemu, for
> example:
> qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
> # win10.img's size: 10G
> But for files do not have any continuous zeros, like compressed files,
> then there will be no any unwritten extents
> unwritten extents are usally seen in very large files
>
> 4. You can fix the typos, My English is not so good
>
> On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
>
> > Hi,
> >
> > The patch looks OK despite the typos and lack of proper formatting on
> > the commit message.
> >
> > But honestly, I don't know if this patch is even mergeable considering
> > you looked at the Linux kernel's source code for this. The patch was
> > already trivial enough
> > if you looked at the documentation and the FreeBSD driver (as I had
> > done in the past but never got to fixing this considering I don't even
> > know if unwritten extents can appear in the wild).
> >
> > I *cannot* stress this enough: Ext4Pkg is a clean room implementation
> > of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
> > compatible with GPLv2) and cannot have random Linux kernel
> > bits, or any other incompatibly-licensed project's bits for that matter.
> >
> > Best regards,
> > Pedro
> >
> >
> >> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
> >>
> >>> From: "Qi Zhou" <atmgnd@outlook.com>
> >>> Subject: [PATCH] unwritten extent suuport
> >>>
> >>> the real lenght of uninitialized/unwritten extent should be (ee_len
> >>> - (1UL << 15)), and
> >>> all related block should been read as zeros. see:
> >>>
> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
> >>>
> >>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
> >>> ---
> >>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
> >>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
> >>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
> >>>  3 files changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> index 070eb5a..7ca8eee 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> @@ -402,6 +402,11 @@ typedef struct {
> >>>
> >>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
> >>>
> >>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
> >>> +
> >>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
> >>> (x - EXTENT_INIT_MAX_LEN)))
> >>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
> >>> +
> >>>  // This on-disk structure is present at the bottom of the extent tree
> >>>  typedef struct {
> >>>    // First logical block
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> index 5fa2fe0..21af573 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> @@ -332,7 +332,7 @@ Ext4GetExtent (
> >>>      return EFI_NO_MAPPING;
> >>>    }
> >>>
> >>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
> >>> Ext->ee_len > LogicalBlock)) {
> >>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
> >>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
> >>>      // This extent does not cover the block
> >>>      if (Buffer != NULL) {
> >>>        FreePool (Buffer);
> >>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
> >>>    Extent = UserStruct;
> >>>    Block  = (UINT32)(UINTN)StandaloneKey;
> >>>
> >>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> >>> Extent->ee_len) {
> >>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> >>> EXTENT_REAL_LEN(Extent->ee_len)) {
> >>>      return 0;
> >>>    }
> >>>
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> index 63cecec..d691ec7 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> @@ -151,6 +151,11 @@ Ext4Read (
> >>>        // Potential improvement: In the future, we could get the
> >>> hole's tota
> >>>        // size and memset all that
> >>>        SetMem (Buffer, WasRead, 0);
> >>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
> >>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block *
> Partition->BlockSize;
> >>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
> >>> Partition->BlockSize - HoleOff;
> >>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
> >>> +      SetMem (Buffer, WasRead, 0);
> >>>      } else {
> >>>        ExtentStartBytes = MultU64x32 (
> >>>                             LShiftU64 (Extent.ee_start_hi, 32) |
> >>> --
> >>> 2.17.1
> >>>
> >
> >
> > --
> >
> > Pedro Falcato
>


-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 8757 bytes --]

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[qi zhou]

This line may do come form linux kernel, As you can see in the first
link I refers says this number (1UL << 15) is kind of magic number. If
you write somethimg linux standanrded, It is hard to keep abosultely no
any linux involued
I think even freebsd has some code from linux, like the second link I
posted, the freebsd's ext4_ext_get_actual_len and EXT_INIT_MAX_LEN are
exactly the same as linux

It is ok if it's considering as not mergeable, I think it is also good
just as a reference on the mailinng list, to those people who need to
read very large files

The debug/fix process, I described here

on the first, I use vbox's ext4 uefi driver to read large files, but
failed on verfication use some tools I writed, I share it here
md5sum.efi: https://1drv.ms/u/s!As-Ec5SPH0fuillwxhIsePY0KBla?e=WzHaBf
diff.efi: https://1drv.ms/u/s!As-Ec5SPH0fuilgMwlg6yNQOFCD1?e=GVoKuH

then I googed to for replacement(on the first, I dont plat to fixed it
myself), But no luck, the all fails on large file read verfication. But
I noticed the performance of edk2-platforms's ext4 driver is most best
of all those uefi ext4 drivers
I did not found a working one, so I need to fix it. First I did some
guess and research, and then I added
some logs dump the edk2's read extents to serial on data that did't not
match, (the diff.efi tool I write will stop reading when data dismatch)
I compare those log dump to linux's 'filefrag -v"'s ouput, It is easy
to found the difference, then I google
to find the logic about unwritten extents, then did the fix


From: Pedro Falcato <pedro.falcato@gmail.com>
Sent: Thursday, October 28, 2021 5:34
To: QiZhou <atmgnd@outlook.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>
Subject: Re: [PATCH][Ext4Pkg] unwritten extent suuport 
 
Hi Qi,

If you didn't use the Linux kernel (nor the documentation) as a reference, can you please tell me what you've used? I'm asking because there's at least a line that's suspiciously similar to Linux's code:

#define EXTENT_INIT_MAX_LEN (1UL << 15)

the UL looks redundant to me, since there's no need for it.

Also, I prefer that you fix the typos yourself and format the patch correctly, including the code.


On Wed, Oct 27, 2021 at 4:45 PM QiZhou <atmgnd@outlook.com> wrote:
1. I am not familiar with freebsd, and don know if freebsd get the same issue,
But I do found the freebsd has some code snippets related to unwritten extent,
see: https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
Is Ext4 is freebsd's default/major file system ?

2. I did't look at linux kernel(ext4) berfor send this patch, I cant
found any offcial document, so I refer to linux source as a standand
when send this patch

3. Yes, unwritten extents are wild used, usally when a file cotains many
zeros, or mark file holes(fallocate, qemu-img...)
You can generate a file contains a lot of unwritten extents by qemu, for
example:
qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
# win10.img's size: 10G
But for files do not have any continuous zeros, like compressed files,
then there will be no any unwritten extents
unwritten extents are usally seen in very large files

4. You can fix the typos, My English is not so good

On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com> wrote:

> Hi,
>  
> The patch looks OK despite the typos and lack of proper formatting on
> the commit message.
>  
> But honestly, I don't know if this patch is even mergeable considering
> you looked at the Linux kernel's source code for this. The patch was
> already trivial enough
> if you looked at the documentation and the FreeBSD driver (as I had
> done in the past but never got to fixing this considering I don't even
> know if unwritten extents can appear in the wild).
>  
> I *cannot* stress this enough: Ext4Pkg is a clean room implementation
> of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
> compatible with GPLv2) and cannot have random Linux kernel
> bits, or any other incompatibly-licensed project's bits for that matter.
>  
> Best regards,
> Pedro
>  
>  
>> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
>>  
>>> From: "Qi Zhou" <atmgnd@outlook.com>
>>> Subject: [PATCH] unwritten extent suuport
>>>  
>>> the real lenght of uninitialized/unwritten extent should be (ee_len
>>> - (1UL << 15)), and
>>> all related block should been read as zeros. see:
>>> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
>>>  
>>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
>>> ---
>>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
>>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
>>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>  
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> index 070eb5a..7ca8eee 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>> @@ -402,6 +402,11 @@ typedef struct {
>>>  
>>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
>>>  
>>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
>>> +
>>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
>>> (x - EXTENT_INIT_MAX_LEN)))
>>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
>>> +
>>>  // This on-disk structure is present at the bottom of the extent tree
>>>  typedef struct {
>>>    // First logical block
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> index 5fa2fe0..21af573 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>> @@ -332,7 +332,7 @@ Ext4GetExtent (
>>>      return EFI_NO_MAPPING;
>>>    }
>>>  
>>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> Ext->ee_len > LogicalBlock)) {
>>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
>>>      // This extent does not cover the block
>>>      if (Buffer != NULL) {
>>>        FreePool (Buffer);
>>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
>>>    Extent = UserStruct;
>>>    Block  = (UINT32)(UINTN)StandaloneKey;
>>>  
>>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> Extent->ee_len) {
>>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>> EXTENT_REAL_LEN(Extent->ee_len)) {
>>>      return 0;
>>>    }
>>>  
>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> index 63cecec..d691ec7 100644
>>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>> @@ -151,6 +151,11 @@ Ext4Read (
>>>        // Potential improvement: In the future, we could get the
>>> hole's tota
>>>        // size and memset all that
>>>        SetMem (Buffer, WasRead, 0);
>>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
>>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block * Partition->BlockSize;
>>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
>>> Partition->BlockSize - HoleOff;
>>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
>>> +      SetMem (Buffer, WasRead, 0);
>>>      } else {
>>>        ExtentStartBytes = MultU64x32 (
>>>                             LShiftU64 (Extent.ee_start_hi, 32) |
>>> --
>>> 2.17.1
>>>  
>  
>  
> --
>  
> Pedro Falcato


-- 
Pedro Falcato

Re: [edk2-devel] [PATCH][Ext4Pkg] unwritten extent suuport

[Kevin@Insyde]

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

Pedro,

I believe he DID reference Linux source

“ 2. I did't look at linux kernel(ext4) berfor send this patch, I cant
found any offcial document, so I refer to linux source as a standand
when send this patch”

Kevin D Davis
Security Strategist
Insyde Software


> On Oct 27, 2021, at 5:43 PM, qi zhou <atmgnd@outlook.com> wrote:
> 
> This line may do come form linux kernel, As you can see in the first
> link I refers says this number (1UL << 15) is kind of magic number. If
> you write somethimg linux standanrded, It is hard to keep abosultely no
> any linux involued
> I think even freebsd has some code from linux, like the second link I
> posted, the freebsd's ext4_ext_get_actual_len and EXT_INIT_MAX_LEN are
> exactly the same as linux
> 
> It is ok if it's considering as not mergeable, I think it is also good
> just as a reference on the mailinng list, to those people who need to
> read very large files
> 
> The debug/fix process, I described here
> 
> on the first, I use vbox's ext4 uefi driver to read large files, but
> failed on verfication use some tools I writed, I share it here
> md5sum.efi: https://1drv.ms/u/s!As-Ec5SPH0fuillwxhIsePY0KBla?e=WzHaBf
> diff.efi: https://1drv.ms/u/s!As-Ec5SPH0fuilgMwlg6yNQOFCD1?e=GVoKuH
> 
> then I googed to for replacement(on the first, I dont plat to fixed it
> myself), But no luck, the all fails on large file read verfication. But
> I noticed the performance of edk2-platforms's ext4 driver is most best
> of all those uefi ext4 drivers
> I did not found a working one, so I need to fix it. First I did some
> guess and research, and then I added
> some logs dump the edk2's read extents to serial on data that did't not
> match, (the diff.efi tool I write will stop reading when data dismatch)
> I compare those log dump to linux's 'filefrag -v"'s ouput, It is easy
> to found the difference, then I google
> to find the logic about unwritten extents, then did the fix
> 
> 
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, October 28, 2021 5:34
> To: QiZhou <atmgnd@outlook.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>
> Subject: Re: [PATCH][Ext4Pkg] unwritten extent suuport 
>  
> Hi Qi,
> 
> If you didn't use the Linux kernel (nor the documentation) as a reference, can you please tell me what you've used? I'm asking because there's at least a line that's suspiciously similar to Linux's code:
> 
> #define EXTENT_INIT_MAX_LEN (1UL << 15)
> 
> the UL looks redundant to me, since there's no need for it.
> 
> Also, I prefer that you fix the typos yourself and format the patch correctly, including the code.
> 
> 
> On Wed, Oct 27, 2021 at 4:45 PM QiZhou <atmgnd@outlook.com> wrote:
> 1. I am not familiar with freebsd, and don know if freebsd get the same issue,
> But I do found the freebsd has some code snippets related to unwritten extent,
> see: https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
> https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
> Is Ext4 is freebsd's default/major file system ?
> 
> 2. I did't look at linux kernel(ext4) berfor send this patch, I cant
> found any offcial document, so I refer to linux source as a standand
> when send this patch
> 
> 3. Yes, unwritten extents are wild used, usally when a file cotains many
> zeros, or mark file holes(fallocate, qemu-img...)
> You can generate a file contains a lot of unwritten extents by qemu, for
> example:
> qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
> # win10.img's size: 10G
> But for files do not have any continuous zeros, like compressed files,
> then there will be no any unwritten extents
> unwritten extents are usally seen in very large files
> 
> 4. You can fix the typos, My English is not so good
> 
>> On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>> 
>> Hi,
>>   
>> The patch looks OK despite the typos and lack of proper formatting on
>> the commit message.
>>   
>> But honestly, I don't know if this patch is even mergeable considering
>> you looked at the Linux kernel's source code for this. The patch was
>> already trivial enough
>> if you looked at the documentation and the FreeBSD driver (as I had
>> done in the past but never got to fixing this considering I don't even
>> know if unwritten extents can appear in the wild).
>>   
>> I *cannot* stress this enough: Ext4Pkg is a clean room implementation
>> of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
>> compatible with GPLv2) and cannot have random Linux kernel
>> bits, or any other incompatibly-licensed project's bits for that matter.
>>   
>> Best regards,
>> Pedro
>>   
>>   
>>>> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
>>>   
>>>> From: "Qi Zhou" <atmgnd@outlook.com>
>>>> Subject: [PATCH] unwritten extent suuport
>>>>   
>>>> the real lenght of uninitialized/unwritten extent should be (ee_len
>>>> - (1UL << 15)), and
>>>> all related block should been read as zeros. see:
>>>> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
>>>>   
>>>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
>>>> ---
>>>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
>>>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
>>>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>>   
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>>> index 070eb5a..7ca8eee 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
>>>> @@ -402,6 +402,11 @@ typedef struct {
>>>>   
>>>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
>>>>   
>>>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
>>>> +
>>>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
>>>> (x - EXTENT_INIT_MAX_LEN)))
>>>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
>>>> +
>>>>  // This on-disk structure is present at the bottom of the extent tree
>>>>  typedef struct {
>>>>    // First logical block
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>>> index 5fa2fe0..21af573 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
>>>> @@ -332,7 +332,7 @@ Ext4GetExtent (
>>>>      return EFI_NO_MAPPING;
>>>>    }
>>>>   
>>>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>>> Ext->ee_len > LogicalBlock)) {
>>>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
>>>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
>>>>      // This extent does not cover the block
>>>>      if (Buffer != NULL) {
>>>>        FreePool (Buffer);
>>>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
>>>>    Extent = UserStruct;
>>>>    Block  = (UINT32)(UINTN)StandaloneKey;
>>>>   
>>>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>>> Extent->ee_len) {
>>>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
>>>> EXTENT_REAL_LEN(Extent->ee_len)) {
>>>>      return 0;
>>>>    }
>>>>   
>>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>>> index 63cecec..d691ec7 100644
>>>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>>>> @@ -151,6 +151,11 @@ Ext4Read (
>>>>        // Potential improvement: In the future, we could get the
>>>> hole's tota
>>>>        // size and memset all that
>>>>        SetMem (Buffer, WasRead, 0);
>>>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
>>>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block * Partition->BlockSize;
>>>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
>>>> Partition->BlockSize - HoleOff;
>>>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
>>>> +      SetMem (Buffer, WasRead, 0);
>>>>      } else {
>>>>        ExtentStartBytes = MultU64x32 (
>>>>                             LShiftU64 (Extent.ee_start_hi, 32) |
>>>> --
>>>> 2.17.1
>>>>   
>>   
>>   
>> --
>>   
>> Pedro Falcato
> 
> 
> -- 
> Pedro Falcato
> 
> 
> 
> 

[-- Attachment #1.2: Type: text/html, Size: 22866 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2199 bytes --]

Re: [PATCH][Ext4Pkg] unwritten extent suuport

[Pedro Falcato]

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

Hi,

Considering the patch is simple enough, it's not worth the risk. I'll
implement support for unwritten extents using the documentation and try to
upstream it together with a bunch of other changes to the driver.

Thanks,
Pedro

On Thu, Oct 28, 2021 at 1:43 AM qi zhou <atmgnd@outlook.com> wrote:

> This line may do come form linux kernel, As you can see in the first
> link I refers says this number (1UL << 15) is kind of magic number. If
> you write somethimg linux standanrded, It is hard to keep abosultely no
> any linux involued
> I think even freebsd has some code from linux, like the second link I
> posted, the freebsd's ext4_ext_get_actual_len and EXT_INIT_MAX_LEN are
> exactly the same as linux
>
> It is ok if it's considering as not mergeable, I think it is also good
> just as a reference on the mailinng list, to those people who need to
> read very large files
>
> The debug/fix process, I described here
>
> on the first, I use vbox's ext4 uefi driver to read large files, but
> failed on verfication use some tools I writed, I share it here
> md5sum.efi: https://1drv.ms/u/s!As-Ec5SPH0fuillwxhIsePY0KBla?e=WzHaBf
> diff.efi: https://1drv.ms/u/s!As-Ec5SPH0fuilgMwlg6yNQOFCD1?e=GVoKuH
>
> then I googed to for replacement(on the first, I dont plat to fixed it
> myself), But no luck, the all fails on large file read verfication. But
> I noticed the performance of edk2-platforms's ext4 driver is most best
> of all those uefi ext4 drivers
> I did not found a working one, so I need to fix it. First I did some
> guess and research, and then I added
> some logs dump the edk2's read extents to serial on data that did't not
> match, (the diff.efi tool I write will stop reading when data dismatch)
> I compare those log dump to linux's 'filefrag -v"'s ouput, It is easy
> to found the difference, then I google
> to find the logic about unwritten extents, then did the fix
>
>
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, October 28, 2021 5:34
> To: QiZhou <atmgnd@outlook.com>
> Cc: devel@edk2.groups.io <devel@edk2.groups.io>
> Subject: Re: [PATCH][Ext4Pkg] unwritten extent suuport
>
> Hi Qi,
>
> If you didn't use the Linux kernel (nor the documentation) as a reference,
> can you please tell me what you've used? I'm asking because there's at
> least a line that's suspiciously similar to Linux's code:
>
> #define EXTENT_INIT_MAX_LEN (1UL << 15)
>
> the UL looks redundant to me, since there's no need for it.
>
> Also, I prefer that you fix the typos yourself and format the patch
> correctly, including the code.
>
>
> On Wed, Oct 27, 2021 at 4:45 PM QiZhou <atmgnd@outlook.com> wrote:
> 1. I am not familiar with freebsd, and don know if freebsd get the same
> issue,
> But I do found the freebsd has some code snippets related to unwritten
> extent,
> see:
> https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.h#L37
>
> https://github.com/freebsd/freebsd-src/blob/b3f46656393f5c8a6e8305afeb5e8c3638025c26/sys/fs/ext2fs/ext2_extents.c#L1347
> Is Ext4 is freebsd's default/major file system ?
>
> 2. I did't look at linux kernel(ext4) berfor send this patch, I cant
> found any offcial document, so I refer to linux source as a standand
> when send this patch
>
> 3. Yes, unwritten extents are wild used, usally when a file cotains many
> zeros, or mark file holes(fallocate, qemu-img...)
> You can generate a file contains a lot of unwritten extents by qemu, for
> example:
> qemu-img convert -f raw -O qcow2 win10.img win10.qcow2
> # win10.img's size: 10G
> But for files do not have any continuous zeros, like compressed files,
> then there will be no any unwritten extents
> unwritten extents are usally seen in very large files
>
> 4. You can fix the typos, My English is not so good
>
> On Oct 27 2021, at 10:56 pm, Pedro Falcato <pedro.falcato@gmail.com>
> wrote:
>
> > Hi,
> >
> > The patch looks OK despite the typos and lack of proper formatting on
> > the commit message.
> >
> > But honestly, I don't know if this patch is even mergeable considering
> > you looked at the Linux kernel's source code for this. The patch was
> > already trivial enough
> > if you looked at the documentation and the FreeBSD driver (as I had
> > done in the past but never got to fixing this considering I don't even
> > know if unwritten extents can appear in the wild).
> >
> > I *cannot* stress this enough: Ext4Pkg is a clean room implementation
> > of ext4 licensed under the BSD-2-Clause-Patent license (which is NOT
> > compatible with GPLv2) and cannot have random Linux kernel
> > bits, or any other incompatibly-licensed project's bits for that matter.
> >
> > Best regards,
> > Pedro
> >
> >
> >> On Wed, Oct 27, 2021 at 2:37 PM qi zhou <atmgnd@outlook.com> wrote:
> >>
> >>> From: "Qi Zhou" <atmgnd@outlook.com>
> >>> Subject: [PATCH] unwritten extent suuport
> >>>
> >>> the real lenght of uninitialized/unwritten extent should be (ee_len
> >>> - (1UL << 15)), and
> >>> all related block should been read as zeros. see:
> >>>
> https://github.com/torvalds/linux/blob/d25f27432f80a800a3592db128254c8140bd71bf/fs/ext4/ext4_extents.h#L156
> >>>
> >>> Signed-off-by: Qi Zhou <atmgnd@outlook.com>
> >>> ---
> >>>  Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 5 +++++
> >>>  Features/Ext4Pkg/Ext4Dxe/Extents.c  | 4 ++--
> >>>  Features/Ext4Pkg/Ext4Dxe/Inode.c    | 5 +++++
> >>>  3 files changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> index 070eb5a..7ca8eee 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> >>> @@ -402,6 +402,11 @@ typedef struct {
> >>>
> >>>  #define EXT4_MIN_DIR_ENTRY_LEN  8
> >>>
> >>> +#define EXTENT_INIT_MAX_LEN (1UL << 15)
> >>> +
> >>> +#define EXTENT_REAL_LEN(x) ((UINT16)(x <= EXTENT_INIT_MAX_LEN ? x :
> >>> (x - EXTENT_INIT_MAX_LEN)))
> >>> +#define EXTENT_IS_UNWRITTEN(x) (x > EXTENT_INIT_MAX_LEN)
> >>> +
> >>>  // This on-disk structure is present at the bottom of the extent tree
> >>>  typedef struct {
> >>>    // First logical block
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> index 5fa2fe0..21af573 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Extents.c
> >>> @@ -332,7 +332,7 @@ Ext4GetExtent (
> >>>      return EFI_NO_MAPPING;
> >>>    }
> >>>
> >>> -  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
> >>> Ext->ee_len > LogicalBlock)) {
> >>> +  if (!(LogicalBlock >= Ext->ee_block && Ext->ee_block +
> >>> EXTENT_REAL_LEN(Ext->ee_len) > LogicalBlock)) {
> >>>      // This extent does not cover the block
> >>>      if (Buffer != NULL) {
> >>>        FreePool (Buffer);
> >>> @@ -413,7 +413,7 @@ Ext4ExtentsMapKeyCompare (
> >>>    Extent = UserStruct;
> >>>    Block  = (UINT32)(UINTN)StandaloneKey;
> >>>
> >>> -  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> >>> Extent->ee_len) {
> >>> +  if (Block >= Extent->ee_block && Block < Extent->ee_block +
> >>> EXTENT_REAL_LEN(Extent->ee_len)) {
> >>>      return 0;
> >>>    }
> >>>
> >>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> index 63cecec..d691ec7 100644
> >>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
> >>> @@ -151,6 +151,11 @@ Ext4Read (
> >>>        // Potential improvement: In the future, we could get the
> >>> hole's tota
> >>>        // size and memset all that
> >>>        SetMem (Buffer, WasRead, 0);
> >>> +    } else if(EXTENT_IS_UNWRITTEN(Extent.ee_len)) {
> >>> +      HoleOff = CurrentSeek - (UINT64)Extent.ee_block *
> Partition->BlockSize;
> >>> +      HoleLen = EXTENT_REAL_LEN(Extent.ee_len) *
> >>> Partition->BlockSize - HoleOff;
> >>> +      WasRead = HoleLen > RemainingRead ? RemainingRead : HoleLen;
> >>> +      SetMem (Buffer, WasRead, 0);
> >>>      } else {
> >>>        ExtentStartBytes = MultU64x32 (
> >>>                             LShiftU64 (Extent.ee_start_hi, 32) |
> >>> --
> >>> 2.17.1
> >>>
> >
> >
> > --
> >
> > Pedro Falcato
>
>
> --
> Pedro Falcato



-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 11633 bytes --]