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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#82761): https://edk2.groups.io/g/devel/message/82761 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] -=-=-=-=-=-=-=-=-=-=-=-
