linux-ext4 - Re: [PATCH 5/6] ext4: fix punch

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: <87fwihi6zi.fsf@dmbot.sw.ru>
Date:	Tue, 25 Oct 2011 16:35:45 +0400
From:	Dmitry Monakhov <dmonakhov@...nvz.org>
To:	Ted Ts'o <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org, achender@...ux.vnet.ibm.com
Subject: Re: [PATCH 5/6] ext4: fix punch_hole extend handler

On Tue, 25 Oct 2011 08:05:53 -0400, "Ted Ts'o" <tytso@....edu> wrote:
> On Fri, Oct 21, 2011 at 01:08:58AM +0400, Dmitry Monakhov wrote:
> > Current implementation has following issues:
> >  - EOFBLOCK does not changed if necessery
> >  - ext4_ext_rm_leaf() may return -EAGAIN due to transaction restart
> >  - punched extent converted to uninitialized incorrectly, i dont understand
> >    why do we ever have to do that conversion, but once we do it let's do it
> >    in a right way.
> >  - fsync() logic is broken because ei->i_sync_tid was not updated
> >  - Last but not the least all: punch hole logic is sited directly
> >    in ext4_ext_map_blocks() on 3rd level of control, IMHO one can
> >    easily screw-up his eyes while invastigating that code. We have
> >    nothing to hide aren't we?
> > 
> > This patch performs following changes:
> >  - Move punch hole logic to didicated function similar to uninitialized
> >    extent handlers.
> >  - Clear EOFBLOCK if necessery, unfortunately we can not reuse
> >    check_eofblock_fl() function because it purpose to handle file
> >    expansion, but in our case we have to recheck base invariant that:
> >       clear_eof_flag = (eof_block >= last_allocated_block)
> >  - Repeat punch hole after transaction restart.
> >  - Update inode sync transaction id on exit.
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
> 
> I'm really sorry, but this was too complicated for me to review for
> this merge window.  Maybe Allison or some of the people who worked on
> the punch code could review this?  Or maybe this could be broken up
> into smaller patches?
Yes. ;( you right, it is kind of non obvious patch.
i'll split this like follows:
1) move punch_hole handler from ext4_ext_map_blocks to dedicated
function as is (90% the original patch)
2) fix other issues one by one.
Will do
> 
> Or I can look at this again for the next merge window, but this is too
> complicated for me to understand (and the punch code is too new for me
> to have an deep understanding for quick reviews).  Sorry I didn't
> review this sooner; between trying to get e2fsprogs 1.42 ready for
> release and kernel.org recovery efforts, I got beind on my reviews.
BTW can you please highlight your plans to make libquota in to working
shape, right now it simply rewrites old quota file so fsck each time
result int fs modifications. I have some patches in my queue which add
quota check capabilities in libquota/e2fsck. 

> 
> Regards,
> 
> 						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html