On Tue 03-12-19 18:40:47, Ritesh Harjani wrote:
> 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.
Yeah, I would prefer to keep it simple. Otherwise you would have a rare
subcase of a rare case meaning that code path will hardly ever get tested
and that's not good for maintainability... Also note that check is not 100%
reliable. There's still a race like:
ext4_page_mkwrite()
block_page_mkwrite()
lock_page(page);
...
-> get_block()
if (inode_dio_count(inode) > 0)
-> false - use ext4_get_block()
iomap_dio_rw()
inode_dio_begin()
filemap_write_and_wait()
-> no dirty page yet -> bails
invalidate_mapping_pages2()
set_page_dirty(page);
unlock_page(page);
-> bails with error because the
page is dirty. Warning is
issued but stale data is still
exposed.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
