| Message ID | 20200928194103.244692-4-mfo@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4/jbd2: data=journal: write-protect pages on transaction commit | expand |
On Sep 28, 2020, at 1:41 PM, Mauricio Faria de Oliveira <mfo@canonical.com> wrote: > > These are two fixes for data journalling required by > the next patch, discovered while testing it. > > First, the optimization to return early if all buffers > are mapped is not appropriate for the next patch: > > The inode _must_ be added to the transaction's list in > data=journal mode (so to write-protect pages on commit) > thus we cannot return early there. > > Second, once that optimization to reduce transactions > was disabled for data=journal mode, more transactions > happened, and occasionally hit this warning message: > 'JBD2: Spotted dirty metadata buffer'. > > Reason is, block_page_mkwrite() will set_buffer_dirty() > before do_journal_get_write_access() that is there to > prevent it. This issue was masked by the optimization. > > So, on data=journal use __block_write_begin() instead. > This also requires page locking and len recalculation. > (see block_page_mkwrite() for implementation details.) > > Finally, as Jan noted there is little sharing between > data=journal and other modes in ext4_page_mkwrite(). > > However, a prototype of ext4_journalled_page_mkwrite() > showed there still would be lots of duplicated lines > (tens of) that didn't seem worth it. > > Thus this patch ends up with an ugly goto to skip all > non-data journalling code (to avoid long indentations, > but that can be changed..) in the beginning, and just > a conditional in the transaction section. > > Well, we skip a common part to data journalling which > is the page truncated check, but we do it again after > ext4_journal_start() when we re-acquire the page lock > (so not to acquire the page lock twice needlessly for > data journalling.) > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> > Suggested-by: Jan Kara <jack@suse.cz> Thanks for the clear commit message. It definitely would not be clear why the patch was structured in this way without it. Reviewed-by: Andreas Dilger <adilger@dilger.ca> Cheers, Andreas
On Mon 28-09-20 16:41:02, Mauricio Faria de Oliveira wrote: > These are two fixes for data journalling required by > the next patch, discovered while testing it. > > First, the optimization to return early if all buffers > are mapped is not appropriate for the next patch: > > The inode _must_ be added to the transaction's list in > data=journal mode (so to write-protect pages on commit) > thus we cannot return early there. > > Second, once that optimization to reduce transactions > was disabled for data=journal mode, more transactions > happened, and occasionally hit this warning message: > 'JBD2: Spotted dirty metadata buffer'. > > Reason is, block_page_mkwrite() will set_buffer_dirty() > before do_journal_get_write_access() that is there to > prevent it. This issue was masked by the optimization. > > So, on data=journal use __block_write_begin() instead. > This also requires page locking and len recalculation. > (see block_page_mkwrite() for implementation details.) > > Finally, as Jan noted there is little sharing between > data=journal and other modes in ext4_page_mkwrite(). > > However, a prototype of ext4_journalled_page_mkwrite() > showed there still would be lots of duplicated lines > (tens of) that didn't seem worth it. > > Thus this patch ends up with an ugly goto to skip all > non-data journalling code (to avoid long indentations, > but that can be changed..) in the beginning, and just > a conditional in the transaction section. > > Well, we skip a common part to data journalling which > is the page truncated check, but we do it again after > ext4_journal_start() when we re-acquire the page lock > (so not to acquire the page lock twice needlessly for > data journalling.) > > Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> > Suggested-by: Jan Kara <jack@suse.cz> The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 7 deletions(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index bf596467c234..ac153e340a6f 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -5977,9 +5977,17 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) > if (err) > goto out_ret; > > + /* > + * On data journalling we skip straight to the transaction handle: > + * there's no delalloc; page truncated will be checked later; the > + * early return w/ all buffers mapped (calculates size/len) can't > + * be used; and there's no dioread_nolock, so only ext4_get_block. > + */ > + if (ext4_should_journal_data(inode)) > + goto retry_alloc; > + > /* Delalloc case is easy... */ > if (test_opt(inode->i_sb, DELALLOC) && > - !ext4_should_journal_data(inode) && > !ext4_nonda_switch(inode->i_sb)) { > do { > err = block_page_mkwrite(vma, vmf, > @@ -6005,6 +6013,9 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) > /* > * Return if we have all the buffers mapped. This avoids the need to do > * journal_start/journal_stop which can block and take a long time > + * > + * This cannot be done for data journalling, as we have to add the > + * inode to the transaction's list to writeprotect pages on commit. > */ > if (page_has_buffers(page)) { > if (!ext4_walk_page_buffers(NULL, page_buffers(page), > @@ -6029,16 +6040,42 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) > ret = VM_FAULT_SIGBUS; > goto out; > } > - err = block_page_mkwrite(vma, vmf, get_block); > - if (!err && ext4_should_journal_data(inode)) { > - if (ext4_walk_page_buffers(handle, page_buffers(page), 0, > - PAGE_SIZE, NULL, do_journal_get_write_access)) { > + /* > + * Data journalling can't use block_page_mkwrite() because it > + * will set_buffer_dirty() before do_journal_get_write_access() > + * thus might hit warning messages for dirty metadata buffers. > + */ > + if (!ext4_should_journal_data(inode)) { > + err = block_page_mkwrite(vma, vmf, get_block); > + } else { > + lock_page(page); > + size = i_size_read(inode); > + /* Page got truncated from under us? */ > + if (page->mapping != mapping || page_offset(page) > size) { > unlock_page(page); > - ret = VM_FAULT_SIGBUS; > + ret = VM_FAULT_NOPAGE; > ext4_journal_stop(handle); > goto out; > } > - ext4_set_inode_state(inode, EXT4_STATE_JDATA); > + > + if (page->index == size >> PAGE_SHIFT) > + len = size & ~PAGE_MASK; > + else > + len = PAGE_SIZE; > + > + err = __block_write_begin(page, 0, len, ext4_get_block); > + if (!err) { > + if (ext4_walk_page_buffers(handle, page_buffers(page), > + 0, len, NULL, do_journal_get_write_access)) { > + unlock_page(page); > + ret = VM_FAULT_SIGBUS; > + ext4_journal_stop(handle); > + goto out; > + } > + ext4_set_inode_state(inode, EXT4_STATE_JDATA); > + } else { > + unlock_page(page); > + } > } > ext4_journal_stop(handle); > if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries)) > -- > 2.17.1 >
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index bf596467c234..ac153e340a6f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -5977,9 +5977,17 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) if (err) goto out_ret; + /* + * On data journalling we skip straight to the transaction handle: + * there's no delalloc; page truncated will be checked later; the + * early return w/ all buffers mapped (calculates size/len) can't + * be used; and there's no dioread_nolock, so only ext4_get_block. + */ + if (ext4_should_journal_data(inode)) + goto retry_alloc; + /* Delalloc case is easy... */ if (test_opt(inode->i_sb, DELALLOC) && - !ext4_should_journal_data(inode) && !ext4_nonda_switch(inode->i_sb)) { do { err = block_page_mkwrite(vma, vmf, @@ -6005,6 +6013,9 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) /* * Return if we have all the buffers mapped. This avoids the need to do * journal_start/journal_stop which can block and take a long time + * + * This cannot be done for data journalling, as we have to add the + * inode to the transaction's list to writeprotect pages on commit. */ if (page_has_buffers(page)) { if (!ext4_walk_page_buffers(NULL, page_buffers(page), @@ -6029,16 +6040,42 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf) ret = VM_FAULT_SIGBUS; goto out; } - err = block_page_mkwrite(vma, vmf, get_block); - if (!err && ext4_should_journal_data(inode)) { - if (ext4_walk_page_buffers(handle, page_buffers(page), 0, - PAGE_SIZE, NULL, do_journal_get_write_access)) { + /* + * Data journalling can't use block_page_mkwrite() because it + * will set_buffer_dirty() before do_journal_get_write_access() + * thus might hit warning messages for dirty metadata buffers. + */ + if (!ext4_should_journal_data(inode)) { + err = block_page_mkwrite(vma, vmf, get_block); + } else { + lock_page(page); + size = i_size_read(inode); + /* Page got truncated from under us? */ + if (page->mapping != mapping || page_offset(page) > size) { unlock_page(page); - ret = VM_FAULT_SIGBUS; + ret = VM_FAULT_NOPAGE; ext4_journal_stop(handle); goto out; } - ext4_set_inode_state(inode, EXT4_STATE_JDATA); + + if (page->index == size >> PAGE_SHIFT) + len = size & ~PAGE_MASK; + else + len = PAGE_SIZE; + + err = __block_write_begin(page, 0, len, ext4_get_block); + if (!err) { + if (ext4_walk_page_buffers(handle, page_buffers(page), + 0, len, NULL, do_journal_get_write_access)) { + unlock_page(page); + ret = VM_FAULT_SIGBUS; + ext4_journal_stop(handle); + goto out; + } + ext4_set_inode_state(inode, EXT4_STATE_JDATA); + } else { + unlock_page(page); + } } ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
These are two fixes for data journalling required by the next patch, discovered while testing it. First, the optimization to return early if all buffers are mapped is not appropriate for the next patch: The inode _must_ be added to the transaction's list in data=journal mode (so to write-protect pages on commit) thus we cannot return early there. Second, once that optimization to reduce transactions was disabled for data=journal mode, more transactions happened, and occasionally hit this warning message: 'JBD2: Spotted dirty metadata buffer'. Reason is, block_page_mkwrite() will set_buffer_dirty() before do_journal_get_write_access() that is there to prevent it. This issue was masked by the optimization. So, on data=journal use __block_write_begin() instead. This also requires page locking and len recalculation. (see block_page_mkwrite() for implementation details.) Finally, as Jan noted there is little sharing between data=journal and other modes in ext4_page_mkwrite(). However, a prototype of ext4_journalled_page_mkwrite() showed there still would be lots of duplicated lines (tens of) that didn't seem worth it. Thus this patch ends up with an ugly goto to skip all non-data journalling code (to avoid long indentations, but that can be changed..) in the beginning, and just a conditional in the transaction section. Well, we skip a common part to data journalling which is the page truncated check, but we do it again after ext4_journal_start() when we re-acquire the page lock (so not to acquire the page lock twice needlessly for data journalling.) Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com> Suggested-by: Jan Kara <jack@suse.cz> --- fs/ext4/inode.c | 51 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-)