Re: [PATCH][Ext4Pkg] unwritten extent suuport

["Pedro Falcato" <pedro.falcato@gmail.com>]

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