Re: [RFCv3 4/4] ext4: Move to shared iolock even without dioread_nolock mount opt

[Ritesh Harjani <riteshh@linux.ibm.com>]

On 12/3/19 6:09 PM, Jan Kara wrote:
> 
> Hello Ritesh!
> 
> On Tue 03-12-19 17:24:44, Ritesh Harjani wrote:
>> On 11/29/19 10:48 PM, Jan Kara wrote:
>>>> Also, I wanted to have some more discussions on this race before
>>>> making the changes.
>>>> But nevertheless, it's the right time to discuss those changes here.
>>>>
>>>>> mmap write instantiating dirty page and then someone starting writeback
>>>>> against that page while DIO read is running still theoretically leading to
>>>>> stale data exposure. Now this patch does not have influence on that race
>>>>> but:
>>>>
>>>> Yes, agreed.
>>>>
>>>>>
>>>>> 1) We need to close the race mentioned above. Maybe we could do that by
>>>>> proactively allocating unwritten blocks for a page being faulted when there
>>>>> is direct IO running against the file - the one who fills holes through
>>>>> mmap write while direct IO is running on the file deserves to suffer the
>>>>> performance penalty...
>>>>
>>>> I was giving this a thought. So even if we try to penalize mmap
>>>> write as you mentioned above, what I am not sure about it, is that, how can
>>>> we reliably detect that the DIO is in progress?
>>>>
>>>> Say even if we try to check for atomic_read(&inode->i_dio_count) in mmap
>>>> ext4_page_mkwrite path, it cannot be reliable unless there is some sort of a
>>>> lock protection, no?
>>>> Because after the check the DIO can still snoop in, right?
>>>
>>> Yes, doing this reliably will need some code tweaking. Also thinking about
>>> this in detail, doing a reliable check in ext4_page_mkwrite() is
>>> somewhat difficult so it will be probably less error-prone to deal with the
>>> race in the writeback path.
>>
>> hmm. But if we don't do in ext4_page_mkwrite, then I am afraid on
>> how to handle nodelalloc scenario. Where we will directly go and
>> allocate block via ext4_get_block() in ext4_page_mkwrite(),
>> as explained below.
>> I guess we may need some tweaking at both places.
> 
> Ok, I forgot to mention that. Yes, the nodelalloc case in
> ext4_page_mkwrite() still needs tweaking. But that is not performance
> sensitive path at all. So we can just have there:

hmm. I was of the opinion that why use unwritten blocks or move
from written to unwritten method while we can still avoid it.

> 
> 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> 		get_block = ext4_get_block_unwritten;
> 	else
> 		get_block = ext4_get_block;
> 

Although adding a function ext4_dio_check_get_block() as described in
previous email is also trivial, which could avoid using unwritten
blocks here when DIO is not in progress.
But if you think it's not worth it, then I will go with your suggestion
here.



> and be done with it. And yes, for inodes using indirect blocks, direct IO
> reads can still theoretically expose data from blocks instantiated by hole
> filling from ext4_page_mkwrite(). But that race has always been there
> regardless of DIO locking and is hardly fixable with that on-disk format.
> 

Agreed.


> 								Honza
> 
>>
>>
>>>
>>> My preferred way of dealing with this would be to move inode_dio_begin()
>>> call in iomap_dio_rw() a bit earlier before page cache invalidation and add
>>> there smp_mb_after_atomic() (so that e.g. nrpages checks cannot get
>>> reordered before the increment).  Then the check on i_dio_count in
>>> ext4_writepages() will be reliable if we do it after gathering and locking
>>> pages for writeback (i.e., in mpage_map_and_submit_extent()) - either we
>>> see i_dio_count elevated and use the safe (but slower) writeback using
>>> unwritten extents, or we see don't and then we are sure DIO will not start
>>> until writeback of the pages we have locked has finished because of
>>> filemap_write_and_wait() call in iomap_dio_rw().
>>>
>>>
>>
>> Thanks for explaining this in detail. I guess I understand this part now
>> Earlier my understanding towards mapping->nrpages was not complete.
>>
>> AFAIU, with your above suggestion the race won't happen for delalloc
>> cases. But what if it is a nodelalloc mount option?
>>
>> Say with above changes i.e. after tweaking iomap_dio_rw() code as you
>> mentioned above. Below race could still happen, right?
>>
>> iomap_dio_rw()					
>> filemap_write_and_wait_range() 			
>> inode_dio_begin()
>> smp_mb__after_atomic()
>> invalidate_inode_pages2_range()				
>> 						ext4_page_mkwrite()
>> 						block_page_mkwrite()
>> 		  				  lock_page()
>> 						  ext4_get_block()
>>
>> ext4_map_blocks()
>> //this will return IOMAP_MAPPED entry
>>
>> submit_bio()
>> // this goes and reads the block
>> // with stale data allocated,
>> // by ext4_page_mkwrite()
>>
>>
>> Now, I am assuming that ext4_get_block() via ext4_page_mkwrite() path
>> may try to create the block for hole then and there itself.
>> And if submit_bio() from DIO path is serviced late i.e. after
>> ext4_get_block() has already allocated block there, then this may expose
>> stale data. Thoughts?
>>
>>
>> So to avoid both such races in delalloc & in nodelalloc path,
>> we should add the checks at both ext4_writepages() & also at
>> ext4_page_mkwrite().
>>
>> For ext4_page_mkwrite(), why don't we just change the "get_block"
>> function pointer which is passed to block_page_mkwrite()
>> as below. This should solve our race since
>> ext4_dio_check_get_block() will be only called with lock_page()
>> held. And also with inode_dio_begin() now moved up before
>> invalidate_inode_pages2_range(), we could be sure
>> about DIO is currently running or not in ext4_page_mkwrite() path.
>>
>> Does this looks correct to you?
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 381813205f99..74c33d03592c 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -806,6 +806,19 @@ int ext4_get_block_unwritten(struct inode *inode,
>> sector_t iblock,
>>   			       EXT4_GET_BLOCKS_IO_CREATE_EXT);
>>   }
>>
>> +int ext4_dio_check_get_block(struct inode *inode, sector_t iblock,
>> +		   struct buffer_head *bh, int create)
>> +{
>> +	get_block_t *get_block;
>> +
>> +	if (!atomic_read(&inode->i_dio_count))
>> +		get_block = ext4_get_block;
>> +	else
>> +		get_block = ext4_get_block_unwritten;
>> +
>> +	return get_block(inode, iblock, bh, create);
>> +}
>> +
>>   /* Maximum number of blocks we map for direct IO at once. */
>>   #define DIO_MAX_BLOCKS 4096
>>
>> @@ -2332,7 +2345,8 @@ static int mpage_map_one_extent(handle_t *handle,
>> struct mpage_da_data *mpd)
>>   	struct inode *inode = mpd->inode;
>>   	struct ext4_map_blocks *map = &mpd->map;
>>   	int get_blocks_flags;
>> -	int err, dioread_nolock;
>> +	int err;
>> +	bool dio_in_progress = atomic_read(&inode->i_dio_count);
>>
>>   	trace_ext4_da_write_pages_extent(inode, map);
>>   	/*
>> @@ -2353,8 +2367,14 @@ static int mpage_map_one_extent(handle_t *handle,
>> struct mpage_da_data *mpd)
>>   	get_blocks_flags = EXT4_GET_BLOCKS_CREATE |
>>   			   EXT4_GET_BLOCKS_METADATA_NOFAIL |
>>   			   EXT4_GET_BLOCKS_IO_SUBMIT;
>> -	dioread_nolock = ext4_should_dioread_nolock(inode);
>> -	if (dioread_nolock)
>> +
>> +	/*
>> +	 * There could be race between DIO read & ext4_page_mkwrite
>> +	 * where in delalloc case, we may go and try to allocate the
>> +	 * block here but if DIO read is in progress then it may expose
>> +	 * stale data, hence use unwritten blocks for allocation
>> +	 * when DIO is in progress.
>> +	 */
>> +	if (dio_in_progress)
>>   		get_blocks_flags |= EXT4_GET_BLOCKS_IO_CREATE_EXT;
>>   	if (map->m_flags & (1 << BH_Delay))
>>   		get_blocks_flags |= EXT4_GET_BLOCKS_DELALLOC_RESERVE;
>> @@ -2362,7 +2382,7 @@ static int mpage_map_one_extent(handle_t *handle,
>> struct mpage_da_data *mpd)
>>   	err = ext4_map_blocks(handle, inode, map, get_blocks_flags);
>>   	if (err < 0)
>>   		return err;
>> -	if (dioread_nolock && (map->m_flags & EXT4_MAP_UNWRITTEN)) {
>> +	if (dio_in_progress && (map->m_flags & EXT4_MAP_UNWRITTEN)) {
>>   		if (!mpd->io_submit.io_end->handle &&
>>   		    ext4_handle_valid(handle)) {
>>   			mpd->io_submit.io_end->handle = handle->h_rsv_handle;
>> @@ -5906,10 +5926,7 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
>>   	}
>>   	unlock_page(page);
>>   	/* OK, we need to fill the hole... */
>> -	if (ext4_should_dioread_nolock(inode))
>> -		get_block = ext4_get_block_unwritten;
>> -	else
>> -		get_block = ext4_get_block;
>> +	get_block = ext4_dio_check_get_block;
>>   retry_alloc:
>>   	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
>>   				    ext4_writepage_trans_blocks(inode));
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index 2f88d64c2a4d..09d0601e5ecb 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -465,6 +465,8 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>>   	if (ret)
>>   		goto out_free_dio;
>>
>> +	inode_dio_begin(inode);
>> +	smp_mb__after_atomic();
>>   	/*
>>   	 * Try to invalidate cache pages for the range we're direct
>>   	 * writing.  If this invalidation fails, tough, the write will
>> @@ -484,8 +486,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>>   			goto out_free_dio;
>>   	}
>>
>> -	inode_dio_begin(inode);
>> -
>>   	blk_start_plug(&plug);
>>   	do {
>>   		ret = iomap_apply(inode, pos, count, flags, ops, dio,
>>
>>
>>
>> -ritesh
>>