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 <atm...@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.falc...@gmail.com> > Sent: Thursday, October 28, 2021 5:34 > To: QiZhou <atm...@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 <atm...@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.falc...@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 <atm...@outlook.com> wrote: > >> > >>> From: "Qi Zhou" <atm...@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 <atm...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#82809): https://edk2.groups.io/g/devel/message/82809 Mute This Topic: https://groups.io/mt/86629612/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
