linux-ext4 - Re: [PATCHv5 5/9] ext2: Move direct-io to use iomap

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] [day] [month] [year] [list]

Message-Id: <87h6taip8u.fsf@doe.com>
Date:   Thu, 20 Apr 2023 20:18:17 +0530
From:   Ritesh Harjani (IBM) <ritesh.list@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        Jan Kara <jack@...e.cz>, Christoph Hellwig <hch@...radead.org>,
        "Darrick J . Wong" <djwong@...nel.org>,
        Ojaswin Mujoo <ojaswin@...ux.ibm.com>,
        Disha Goel <disgoel@...ux.ibm.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCHv5 5/9] ext2: Move direct-io to use iomap

Jan Kara <jack@...e.cz> writes:

> On Sun 16-04-23 15:38:40, Ritesh Harjani (IBM) wrote:
>> This patch converts ext2 direct-io path to iomap interface.
>> - This also takes care of DIO_SKIP_HOLES part in which we return -ENOTBLK
>>   from ext2_iomap_begin(), in case if the write is done on a hole.
>> - This fallbacks to buffered-io in case of DIO_SKIP_HOLES or in case of
>>   a partial write or if any error is detected in ext2_iomap_end().
>>   We try to return -ENOTBLK in such cases.
>> - For any unaligned or extending DIO writes, we pass
>>   IOMAP_DIO_FORCE_WAIT flag to ensure synchronous writes.
>> - For extending writes we set IOMAP_F_DIRTY in ext2_iomap_begin because
>>   otherwise with dsync writes on devices that support FUA, generic_write_sync
>>   won't be called and we might miss inode metadata updates.
>> - Since ext2 already now uses _nolock vartiant of sync write. Hence
>>   there is no inode lock problem with iomap in this patch.
>> - ext2_iomap_ops are now being shared by DIO, DAX & fiemap path
>>
>> Tested-by: Disha Goel <disgoel@...ux.ibm.com>
>> Reviewed-by: Christoph Hellwig <hch@....de>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>
> One comment below:
>
>> @@ -844,6 +868,13 @@ static int
>>  ext2_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>>  		ssize_t written, unsigned flags, struct iomap *iomap)
>>  {
>> +	/*
>> +	 * Switch to buffered-io in case of any error.
>> +	 * Blocks allocated can be used by the buffered-io path.
>> +	 */
>> +	if ((flags & IOMAP_DIRECT) && (flags & IOMAP_WRITE) && written == 0)
>> +		return -ENOTBLK;
>> +
>>  	if (iomap->type == IOMAP_MAPPED &&
>>  	    written < length &&
>>  	    (flags & IOMAP_WRITE))
>
> Is this really needed? What for?
>

Sorry Jan, I got caught into something else so couldn't respond on this
earlier. Thanks a lot for review.

I don't think this will be called for IOMAP_DIRECT for write case.
I mostly see this code was already present for IOMAP_DAX path.
It is to truncate the blocks in case if the iomap dax write failed to
write but the blocks might have been allocated in ->iomap_begin
function.

Is there a specific query that you would like me to check and verify?
I can check more by probing this path to see what happens when this gets
called. But my understanding was it is used for truncating blocks as I
mentioned above.

Thanks
-ritesh