Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201027132751.29858-1-jack@suse.cz> <CAO9xwp0AtCLG77g6fWgu9un9XPD3d5U6ZtjWc3FRJrB8NK44SQ@mail.gmail.com>
 <CAO9xwp3sSjzy9W8pMjV6vYitfZ9BmZE-9bLwcLg1uz3CFBHUcQ@mail.gmail.com> <20201123093434.GA27294@quack2.suse.cz>
In-Reply-To: <20201123093434.GA27294@quack2.suse.cz>
From:   Mauricio Faria de Oliveira <mfo@canonical.com>
Date:   Mon, 23 Nov 2020 21:03:06 -0300
Message-ID: <CAO9xwp3hj2dbAxhd2azayTinfw7pWY9OM43EgW09=btjpYT7Vg@mail.gmail.com>
Subject: Re: [PATCH] ext4: Fix mmap write protection for data=journal mode
To:     Jan Kara <jack@suse.cz>
Cc:     Ted Tso <tytso@mit.edu>, linux-ext4@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Jan,

On Mon, Nov 23, 2020 at 6:34 AM Jan Kara <jack@suse.cz> wrote:
> > ...
> >
> > The remaining checksum changes due to write-protect failures _seem_ to be
> > a race between our write-protect with write_cache_pages() and writeback/sync.
> > But I'm not exactly sure as it's been hard to find the timing/steps
> > for both threads.
> > The idea is,
> >
> > Our write_cache_pages() during commit /
> > ext4_journalled_submit_inode_data_buffers()
> > depends on PAGECACHE_TAG_DIRTY being set, for pagevec_lookup_range_tag()
> > to put such pages in the array to be write-protected with
> > clear_page_dirty_for_io().
> >
> > With a debug patch to dump_stack() callers of
> > __test_set_page_writeback(), that can
> > xas_clear_mark() it, _while_ the page is still going in our call to
> > write_cache_pages(),
> > we see this: wb_workfn() -> ext4_writepage() -> ext4_ext4_bio_write_page(),
>
> I guess there was something between wb_workfn() and ext4_writepage(),
> wasn't there? There should be write_cache_pages()...
>

Yes, you're right, I oversimplified the function path between the first two. :)
(I'll use '...' in the future to make that clearer.)

> > i.e., _not_ going through ext4_journalled_writepage(), which
> > knows/waits on journal.
> > The other leg of ext4_writepage()  _doesn't_, and thus can clear the
> > tag and prevent
> > write-protect while the journal commit / our write_cache_pages() is running.
>
> So I don't think this is quite it. If there are two writebacks racing,
> either of them will writeprotect the page which is enough for commit to be
> safe (as the mapped write will then trigger ext4_page_mkwrite() which will
> wait for the end of running commit in jbd2_journal_get_write_access()). Or
> am I missing something? But there must be still some race as you can still
> see occasional checksum changes... So I must be missing something.
>

Actually you're right, I missed that -- either writeback will
writeprotect the page.
There's still something else going on, but it's me not you who missed
something. :)
Thanks for pointing that out.

> > Since the switch to either leg is PageChecked(), I guess this might be
> > due to clearing it right away in ext4_journalled_writepage(), before
> > write-protection.
>
> The write-protection happens in clear_page_dirty_for_io() call so that's
> before our ->writepage() callback is even called.
>

Right, I meant write-protection by the other/racing writeback thread.
But please nevermind, as this doesn't seem to be the corner case anymore.

Thanks!

>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR


-- 
Mauricio Faria de Oliveira