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

["Kevin@Insyde" <kevin.davis@insyde.com>]

[-- 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 --]