linux-ext4 - Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap infrastructure

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: <20190917101220.3568352052@d06av21.portsmouth.uk.ibm.com>
Date:   Tue, 17 Sep 2019 15:42:19 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Matthew Bobrowski <mbobrowski@...browski.org>, tytso@....edu,
        jack@...e.cz, adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, david@...morbit.com,
        darrick.wong@...cle.com
Subject: Re: [PATCH v3 5/6] ext4: introduce direct IO write path using iomap
 infrastructure

On 9/17/19 2:32 PM, Christoph Hellwig wrote:
> On Tue, Sep 17, 2019 at 02:30:15PM +0530, Ritesh Harjani wrote:
>> So if we have a delayed buffered write to a file,
>> in that case we first only update inode->i_size and update
>> i_disksize at writeback time
>> (i.e. during block allocation).
>> In that case when we call for ext4_dio_write_iter
>> since offset + len > i_disksize, we call for ext4_update_i_disksize().
>>
>> Now if writeback for some reason failed. And the system crashes, during the
>> DIO writes, after the blocks are allocated. Then during reboot we may have
>> an inconsistent inode, since we did not add the inode into the
>> orphan list before we updated the inode->i_disksize. And journal replay
>> may not succeed.
>>
>> 1. Can above actually happen? I am still not able to figure out the
>>     race/inconsistency completely.
>> 2. Can you please help explain under what other cases
>>     it was necessary to call ext4_update_i_disksize() in DIO write paths?
>> 3. When will i_disksize be out-of-sync with i_size during DIO writes?
> 
> None of the above seems new in this patchset, does it?  That being said

In original code before updating i_disksize in ext4_direct_IO_write,
we used to add the inode into the orphan list (which will mark the iloc
dirty and also update the ondisk inode size). Only then we update the 
i_disksize to inode->i_size (which still I don't understand the
reason to put inside open journal handle).

So in case if the crash happens, then in the recovery, we can replay the
journal and we truncate any extra blocks beyond i_size.
(ext4_orphan_cleanup()).

In new iomap implementation (i.e. this patchset), we are doing this in
reverse.

We first call for ext4_update_i_disksize() in ext4_dio_write_iter(),
and then in ext4_iomap_begin() after ext4_map_blocks(),
we add the inode to orphan list, which I am not really sure whether it 
is really consistent with on disk size??

> I found the early size update odd.  XFS updates the on-disk size only
> at I/O completion time to deal with various races including the
> potential exposure of stale data.
> 

Yes, can't really say why it is the case in ext4.
That's mostly what I wanted to understand from previous queries.

-ritesh