linux-kernel - Re: [f2fs-dev] [PATCH] f2fs: simplify accounting inflight directIO request

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: <e1b61b7f-a492-f28b-8a98-557e81a2022a@kernel.org>
Date:   Fri, 23 Jul 2021 07:14:57 +0800
From:   Chao Yu <chao@...nel.org>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     jaegeuk@...nel.org, Chao Yu <chao.yu@...ux.dev>,
        linux-kernel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: simplify accounting inflight directIO
 request

On 2021/7/22 23:48, Eric Biggers wrote:
> On Thu, Jul 22, 2021 at 09:16:17PM +0800, Chao Yu wrote:
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index ba120d55e9b1..d0a1ca6ae38e 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1720,6 +1720,9 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
>>   		map_bh(bh, inode->i_sb, map.m_pblk);
>>   		bh->b_state = (bh->b_state & ~F2FS_MAP_FLAGS) | map.m_flags;
>>   		bh->b_size = blks_to_bytes(inode, map.m_len);
>> +
>> +		if (flag == F2FS_GET_BLOCK_DIO)
>> +			bh->b_private = (void *)may_write;
> 
> Why is this hunk needed?

For passing rw info from get_block() to dio->private.

> 
>> +static int f2fs_dio_end_io(struct kiocb *iocb, loff_t offset,
>> +					ssize_t bytes, void *private)
>>   {
>> -	struct f2fs_private_dio *dio = bio->bi_private;
>> -
>> -	dec_page_count(F2FS_I_SB(dio->inode),
>> -			dio->write ? F2FS_DIO_WRITE : F2FS_DIO_READ);
>> -
>> -	bio->bi_private = dio->orig_private;
>> -	bio->bi_end_io = dio->orig_end_io;
>> -
>> -	kfree(dio);
>> +	struct inode *inode = file_inode(iocb->ki_filp);
>> +	bool may_write = private;
>>   
>> -	bio_endio(bio);
>> +	dec_dio_req_count(F2FS_I_SB(inode), may_write ? WRITE : READ);
>> +	return 0;
>>   }
>>   
>>   static void f2fs_dio_submit_bio(struct bio *bio, struct inode *inode,
>>   							loff_t file_offset)
>>   {
>> -	struct f2fs_private_dio *dio;
>> -	bool write = (bio_op(bio) == REQ_OP_WRITE);
>> -
>> -	dio = f2fs_kzalloc(F2FS_I_SB(inode),
>> -			sizeof(struct f2fs_private_dio), GFP_NOFS);
>> -	if (!dio)
>> -		goto out;
>> -
>> -	dio->inode = inode;
>> -	dio->orig_end_io = bio->bi_end_io;
>> -	dio->orig_private = bio->bi_private;
>> -	dio->write = write;
>> -
>> -	bio->bi_end_io = f2fs_dio_end_io;
>> -	bio->bi_private = dio;
>> -
>> -	inc_page_count(F2FS_I_SB(inode),
>> -			write ? F2FS_DIO_WRITE : F2FS_DIO_READ);
>> +	inc_dio_req_count(F2FS_I_SB(inode),
>> +			op_is_write(bio_op(bio)) ? WRITE : READ);
>>   
>>   	submit_bio(bio);
>> -	return;
>> -out:
>> -	bio->bi_status = BLK_STS_IOERR;
>> -	bio_endio(bio);
>>   }
> 
> The inc and dec here aren't correctly paired, since f2fs_dio_submit_bio() is
> called once per bio whereas f2fs_dio_end_io() is called when the entire direct
> I/O request (which may have consisted of multiple bios) has completed.

Correct, it still needs to hook bio->bi_end_io rather than hooking dio->end_io,
however, w/o one extra allocated memory, we have no way to pass sbi to
f2fs_dio_end_io() for dec_dio_req_count(sbi, ...) invoking.

> 
> The correct way to do this would be to do the inc before calling
> __blockdev_direct_IO(), and do the dec in end_io or if __blockdev_direct_IO()
> returned without actually issuing any I/O.
> 
> But I think you shouldn't bother with this part of the change before we switch
> to iomap, as it will then need to be changed again anyway.

Agreed.

Thanks,

> 
> - Eric
>