On Thu 20-08-20 19:55:05, Mauricio Faria de Oliveira wrote: > On Wed, Aug 19, 2020 at 7:41 AM Jan Kara <jack@xxxxxxx> wrote: > > > > On Wed 19-08-20 10:44:21, Jan Kara wrote: > > > I was thinking about this and we may need to do this somewhat differently. > > > I've realized that there's the slight trouble that we now use page dirty > > > bit for two purposes in data=journal mode - to track pages that need write > > > protection during commit and also to track pages which have buffers that > > > need checkpointing. And this mixing is making things complex. So I was > > > thinking that we could simply leave PageDirty bit for checkpointing > > > purposes and always make sure buffers are appropriately attached to a > > > transaction as dirty in ext4_page_mkwrite(). [snip] > > > [snip] Furthermore I > > > don't think that the tricks with PageChecked logic we play in data=journal > > > mode are really needed as well which should bring further simplifications. > > > I'll try to code this cleanup. > > > > I was looking more into this but it isn't as simple as I thought because > > get_user_pages() users can still modify data and call set_page_dirty() when > > the page is no longer writeably mapped. And by the time set_page_dirty() is > > called page buffers are not necessarily part of any transaction so we need > > to do effectively what's in ext4_journalled_writepage(). To handle this > > corner case I didn't find anything considerably simpler than the current > > code. > > > > So let's stay with what we have in > > ext4_journalled_submit_inode_data_buffers(), we just have to also redirty > > the page if we find any dirty buffers. > > > > Could you please clarify/comment whether the dirty buffers "flags" are > different between the suggestions for ext4_page_mkwrite() and > ext4_journalled_submit_inode_data_buffers() ? > > I'm asking because.. > > In ext4_page_mkwrite() the suggestion is to attach buffers as dirty to > a transaction, which I guess can be done with > ext4_walk_page_buffers(..., write_end_fn) after > ext4_walk_page_buffers(..., do_journal_get_write_access) -- just as > done in ext4_journalled_writepage() -- and that sets the buffer as > *jbd* dirty (BH_JBDDirty.) Correct. > In ext4_journalled_submit_inode_data_buffers() the suggestion is to > check for dirty buffers to redirty the page > (for the case of buffers that need checkpointing) and I think this is > the non-jbd/just dirty (BH_Dirty.) Again correct :). > If I actually understood your explanation/suggest, the dirty buffer > flags should be different, > as otherwise we'd be unconditionally setting buffers dirty on > ext4_page_mkwrite() to later > check for (known to be) dirty buffers in > ext4_journalled_submit_inode_data_buffers(). > > ... > > And as you mentioned no cleanup / keeping ext4_journalled_writepage() > and the PageChecked bit, > I would like to revisit two questions from the cover letter that would > have no impact with the cleanup, > so to confirm my understanding for the next steps. > > > 3) When checking to redirty the page in the writepage callback, > > does a buffer without a journal head means we should redirty > > the page? (for the reason it's not part of the committing txn) > > Per your explanation about the page dirty bit for buffers that need > checkpointing, I see we cannot redirty > the page just because a buffer isn't part of the transaction -- the > buffer has to be dirty -- so I think it falls > down to your suggestion of 'also redirty if we find any dirty buffers' > (regardless of a buffer w/out txns.) right? Correct. It should be: if (buffer_dirty(bh) || (jh && ...)) redirty > > 4) Should we clear the PageChecked bit? > ... > > Should we try to prevent that [ext4_journalled_writepage() > running later] by, say, clearing the pagechecked bit > > in case we don't have to redirty the page (in the writepage callback) ? > > And I think the answer is no, per your explanation about page dirty > being set elsewhere outside of our control, > and thus ext4_journalled_page() still needs to run, and thus the page > checked bit still needs to remain set; correct? Correct. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
