linux-ext4 - Re: [PATCH 06/15] ext2fs: add new APIs needed for fast commits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD+ocbxTHNDwqyucTif7n65pgiapTH1Exgh6F7fJQNDSkmXEcg@mail.gmail.com>
Date:   Wed, 9 Dec 2020 17:45:27 -0800
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 06/15] ext2fs: add new APIs needed for fast commits

I see that makes sense. In that case, I'll rename the function to
errcode_t ext2fs_decode_extent(struct ext2fs_extent *dst, void *src).
I wonder if it's okay if we make this function return an error in case
the on-disk format is not sane. If we do that, we can add
ext2fs_validate_extent() later. Does that make sense?

On Wed, Dec 2, 2020 at 10:45 AM Theodore Y. Ts'o <tytso@....edu> wrote:
>
> On Fri, Nov 20, 2020 at 11:15:57AM -0800, Harshad Shirwadkar wrote:
> > This patch adds the following new APIs:
> >
> > Count the total number of blocks occupied by inode including
> > intermediate extent tree nodes.
> > extern blk64_t ext2fs_count_blocks(ext2_filsys fs, ext2_ino_t ino,
> >                                        struct ext2_inode *inode);
> >
> > Convert ext3_extent to ext2fs_extent.
> > extern void ext2fs_convert_extent(struct ext2fs_extent *to,
> >                                        struct ext3_extent *from);
>
> So one of the reasons why I've intentionally never exposed "struct
> ext3_extent" in the libext2fs interface is because that's an on-disk
> structure which I keep hoping we might change someday --- for example,
> to allow for 64-bit logical block numbers so we can create ext4 files
> greater than 2**32 blocks.  It might be that some other future
> enhancement, such as say, reflinks (depending on how we implement
> them), or reverse pointers, might also require making changes to the
> on-disk format.
>
> The kernel code has the on-disk format and the various logical
> manipulations of the extent tree hopelessly entangled with each other,
> which means changing the kernel code to support more than one on-disk
> extent structure is going to be **hard**.  But in the userspace code,
> all of the knowledge about the on-disk structure is abstracted away
> inside lib/ext2fs/extent.c.
>
> It may very well be that for fast commit, we're going to need to crack
> open that abstraction barrier a bit.  But let's make sure the function
> name makes it clear that what we are doing here is converting between
> a particular on-disk encoding and the ext2fs abtract extent type.
> "ext2fs_convert_extent" doesn't exactly make this clear.
>
> It might also be that what should do is include a pointer to the fs
> and inode structures, and call this something like
> "ext2fs_{decode,encode}_extent()", and pass in the on-disk format via
> a void *.  We might also want to have some kind of
> ext2fs_validate_extent() function which takes a void * and validates
> the on-disk structure to make sure it's sane.
>
> What do you think?
>
>                                         - Ted