From d143b8c15b9f066e7990d22802e0495891792b8e Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Mon, 8 Jun 2015 12:46:26 -0400 Subject: [PATCH] add patch more-simplifications-in-do_get_write_access --- more-simplifications-in-do_get_write_access | 173 ++++++++++++++++++++++++++++ series | 1 + timestamps | 7 +- 3 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 more-simplifications-in-do_get_write_access diff --git a/more-simplifications-in-do_get_write_access b/more-simplifications-in-do_get_write_access new file mode 100644 index 00000000..f7754fe5 --- /dev/null +++ b/more-simplifications-in-do_get_write_access @@ -0,0 +1,173 @@ +jbd2: more simplifications in do_get_write_access() + +From: Jan Kara + +Check for the simple case of unjournaled buffer first, handle it and +bail out. This allows us to remove one if and unindent the difficult case +by one tab. The result is easier to read. + +Signed-off-by: Jan Kara +Signed-off-by: Theodore Ts'o +--- + fs/jbd2/transaction.c | 130 +++++++++++++++++++++++--------------------------- + 1 file changed, 59 insertions(+), 71 deletions(-) + +diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c +index 1995ea539e96..5207825d1038 100644 +--- a/fs/jbd2/transaction.c ++++ b/fs/jbd2/transaction.c +@@ -893,6 +893,20 @@ repeat: + jh->b_modified = 0; + + /* ++ * If the buffer is not journaled right now, we need to make sure it ++ * doesn't get written to disk before the caller actually commits the ++ * new data ++ */ ++ if (!jh->b_transaction) { ++ JBUFFER_TRACE(jh, "no transaction"); ++ J_ASSERT_JH(jh, !jh->b_next_transaction); ++ JBUFFER_TRACE(jh, "file as BJ_Reserved"); ++ spin_lock(&journal->j_list_lock); ++ __jbd2_journal_file_buffer(jh, transaction, BJ_Reserved); ++ spin_unlock(&journal->j_list_lock); ++ goto done; ++ } ++ /* + * If there is already a copy-out version of this buffer, then we don't + * need to make another one + */ +@@ -903,84 +917,58 @@ repeat: + goto done; + } + +- /* Is there data here we need to preserve? */ ++ JBUFFER_TRACE(jh, "owned by older transaction"); ++ J_ASSERT_JH(jh, jh->b_next_transaction == NULL); ++ J_ASSERT_JH(jh, jh->b_transaction == journal->j_committing_transaction); + +- if (jh->b_transaction && jh->b_transaction != transaction) { +- JBUFFER_TRACE(jh, "owned by older transaction"); +- J_ASSERT_JH(jh, jh->b_next_transaction == NULL); +- J_ASSERT_JH(jh, jh->b_transaction == +- journal->j_committing_transaction); ++ /* ++ * There is one case we have to be very careful about. If the ++ * committing transaction is currently writing this buffer out to disk ++ * and has NOT made a copy-out, then we cannot modify the buffer ++ * contents at all right now. The essence of copy-out is that it is ++ * the extra copy, not the primary copy, which gets journaled. If the ++ * primary copy is already going to disk then we cannot do copy-out ++ * here. ++ */ ++ if (buffer_shadow(bh)) { ++ JBUFFER_TRACE(jh, "on shadow: sleep"); ++ jbd_unlock_bh_state(bh); ++ wait_on_bit_io(&bh->b_state, BH_Shadow, TASK_UNINTERRUPTIBLE); ++ goto repeat; ++ } + +- /* There is one case we have to be very careful about. +- * If the committing transaction is currently writing +- * this buffer out to disk and has NOT made a copy-out, +- * then we cannot modify the buffer contents at all +- * right now. The essence of copy-out is that it is the +- * extra copy, not the primary copy, which gets +- * journaled. If the primary copy is already going to +- * disk then we cannot do copy-out here. */ +- +- if (buffer_shadow(bh)) { +- JBUFFER_TRACE(jh, "on shadow: sleep"); ++ /* ++ * Only do the copy if the currently-owning transaction still needs it. ++ * If buffer isn't on BJ_Metadata list, the committing transaction is ++ * past that stage (here we use the fact that BH_Shadow is set under ++ * bh_state lock together with refiling to BJ_Shadow list and at this ++ * point we know the buffer doesn't have BH_Shadow set). ++ * ++ * Subtle point, though: if this is a get_undo_access, then we will be ++ * relying on the frozen_data to contain the new value of the ++ * committed_data record after the transaction, so we HAVE to force the ++ * frozen_data copy in that case. ++ */ ++ if (jh->b_jlist == BJ_Metadata || force_copy) { ++ JBUFFER_TRACE(jh, "generate frozen data"); ++ if (!frozen_buffer) { ++ JBUFFER_TRACE(jh, "allocate memory for buffer"); + jbd_unlock_bh_state(bh); +- wait_on_bit_io(&bh->b_state, BH_Shadow, +- TASK_UNINTERRUPTIBLE); +- goto repeat; +- } +- +- /* +- * Only do the copy if the currently-owning transaction still +- * needs it. If buffer isn't on BJ_Metadata list, the +- * committing transaction is past that stage (here we use the +- * fact that BH_Shadow is set under bh_state lock together with +- * refiling to BJ_Shadow list and at this point we know the +- * buffer doesn't have BH_Shadow set). +- * +- * Subtle point, though: if this is a get_undo_access, +- * then we will be relying on the frozen_data to contain +- * the new value of the committed_data record after the +- * transaction, so we HAVE to force the frozen_data copy +- * in that case. +- */ +- if (jh->b_jlist == BJ_Metadata || force_copy) { +- JBUFFER_TRACE(jh, "generate frozen data"); ++ frozen_buffer = jbd2_alloc(jh2bh(jh)->b_size, GFP_NOFS); + if (!frozen_buffer) { +- JBUFFER_TRACE(jh, "allocate memory for buffer"); +- jbd_unlock_bh_state(bh); +- frozen_buffer = +- jbd2_alloc(jh2bh(jh)->b_size, +- GFP_NOFS); +- if (!frozen_buffer) { +- printk(KERN_ERR +- "%s: OOM for frozen_buffer\n", +- __func__); +- JBUFFER_TRACE(jh, "oom!"); +- error = -ENOMEM; +- goto out; +- } +- goto repeat; ++ printk(KERN_ERR "%s: OOM for frozen_buffer\n", ++ __func__); ++ JBUFFER_TRACE(jh, "oom!"); ++ error = -ENOMEM; ++ goto out; + } +- jh->b_frozen_data = frozen_buffer; +- frozen_buffer = NULL; +- jbd2_freeze_jh_data(jh); ++ goto repeat; + } +- jh->b_next_transaction = transaction; +- } +- +- +- /* +- * Finally, if the buffer is not journaled right now, we need to make +- * sure it doesn't get written to disk before the caller actually +- * commits the new data +- */ +- if (!jh->b_transaction) { +- JBUFFER_TRACE(jh, "no transaction"); +- J_ASSERT_JH(jh, !jh->b_next_transaction); +- JBUFFER_TRACE(jh, "file as BJ_Reserved"); +- spin_lock(&journal->j_list_lock); +- __jbd2_journal_file_buffer(jh, transaction, BJ_Reserved); +- spin_unlock(&journal->j_list_lock); ++ jh->b_frozen_data = frozen_buffer; ++ frozen_buffer = NULL; ++ jbd2_freeze_jh_data(jh); + } ++ jh->b_next_transaction = transaction; + + done: + jbd_unlock_bh_state(bh); +-- +2.1.4 + + diff --git a/series b/series index 79f02ea5..b01fcf5a 100644 --- a/series +++ b/series @@ -36,6 +36,7 @@ crypto-fix-sparse-warnings simplify-code-flow-in-do_get_write_access simplify-error-path-on-allocation-failure-in-do_get_write-access +more-simplifications-in-do_get_write_access ########################################## # unstable patches diff --git a/timestamps b/timestamps index 1402631a..c8d09581 100755 --- a/timestamps +++ b/timestamps @@ -55,6 +55,7 @@ touch -d @1433779152 BUG_ON-assertion-repeated-for-inode1-not-done-for-inode2 touch -d @1433780601 crypto-fix-sparse-warnings touch -d @1433781547 simplify-code-flow-in-do_get_write_access touch -d @1433781639 simplify-error-path-on-allocation-failure-in-do_get_write-access -touch -d @1433781718 series -touch -d @1433781736 status -touch -d @1433781741 timestamps +touch -d @1433781861 more-simplifications-in-do_get_write_access +touch -d @1433781887 series +touch -d @1433781891 status +touch -d @1433781979 timestamps -- 2.11.4.GIT