| Message ID | 20190809124233.13277-6-jack@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | jbd2: Bit spinlock conversions | expand |
On Fri, Aug 09, 2019 at 02:42:31PM +0200, Jan Kara wrote: > @@ -1660,10 +1660,9 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) > __jbd2_journal_file_buffer(jh, transaction, BJ_Forget); > spin_unlock(&journal->j_list_lock); > } > - > +drop: > jbd_unlock_bh_state(bh); > __brelse(bh); > -drop: > if (drop_reserve) { > /* no need to reserve log space for this block -bzzz */ > handle->h_buffer_credits++; (Workflow observation: this is an example where Gerrit can be *so* *much* *better* than e-mail review at times; sometimes, you *really* want to see more context than what e-mail affords you.) After this patch, the resulting code looks like this: ----- drop: jbd_unlock_bh_state(bh); __brelse(bh); if (drop_reserve) { /* no need to reserve log space for this block -bzzz */ handle->h_buffer_credits++; } return err; not_jbd: jbd_unlock_bh_state(bh); __bforget(bh); goto drop; ---- And we still have a case we jump to not_jbd, at which point hilarity will ensue. This is cleaned up in the following patch in this sequence, but this leaves us in a not-great state if we are ever bisecting. - Ted
On Mon, Oct 28, 2019 at 11:28:08AM -0400, Theodore Y. Ts'o wrote: > drop: > jbd_unlock_bh_state(bh); > __brelse(bh); > if (drop_reserve) { > /* no need to reserve log space for this block -bzzz */ > handle->h_buffer_credits++; > } > return err; > > not_jbd: > jbd_unlock_bh_state(bh); > __bforget(bh); > goto drop; > ---- > > And we still have a case we jump to not_jbd, at which point hilarity > will ensue. > > This is cleaned up in the following patch in this sequence, but this > leaves us in a not-great state if we are ever bisecting. Proposed fixup: I'll apply the following on top of this commit, and then fix the merge conflicts in 6/7 so that the end result looks the same as before. Jan, any objections? I figure this way it'll save you from resending the patch series, since the rest of it looks fine to me. - Ted diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index f2af4afc690a..c7c9a42451c7 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -1541,8 +1541,11 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) jbd_lock_bh_state(bh); - if (!buffer_jbd(bh)) - goto not_jbd; + if (!buffer_jbd(bh)) { + jbd_unlock_bh_state(bh); + __bforget(bh); + return 0; + } jh = bh2jh(bh); /* Critical error: attempting to delete a bitmap buffer, maybe? @@ -1671,11 +1674,6 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) handle->h_buffer_credits++; } return err; - -not_jbd: - jbd_unlock_bh_state(bh); - __bforget(bh); - goto drop; } /**
On Mon 28-10-19 12:01:36, Theodore Y. Ts'o wrote: > On Mon, Oct 28, 2019 at 11:28:08AM -0400, Theodore Y. Ts'o wrote: > > drop: > > jbd_unlock_bh_state(bh); > > __brelse(bh); > > if (drop_reserve) { > > /* no need to reserve log space for this block -bzzz */ > > handle->h_buffer_credits++; > > } > > return err; > > > > not_jbd: > > jbd_unlock_bh_state(bh); > > __bforget(bh); > > goto drop; > > ---- > > > > And we still have a case we jump to not_jbd, at which point hilarity > > will ensue. > > > > This is cleaned up in the following patch in this sequence, but this > > leaves us in a not-great state if we are ever bisecting. > > Proposed fixup: I'll apply the following on top of this commit, and > then fix the merge conflicts in 6/7 so that the end result looks the > same as before. > > Jan, any objections? I figure this way it'll save you from resending > the patch series, since the rest of it looks fine to me. Yeah, looks good to me. Thanks for the fixup! Honza > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index f2af4afc690a..c7c9a42451c7 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -1541,8 +1541,11 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) > > jbd_lock_bh_state(bh); > > - if (!buffer_jbd(bh)) > - goto not_jbd; > + if (!buffer_jbd(bh)) { > + jbd_unlock_bh_state(bh); > + __bforget(bh); > + return 0; > + } > jh = bh2jh(bh); > > /* Critical error: attempting to delete a bitmap buffer, maybe? > @@ -1671,11 +1674,6 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) > handle->h_buffer_credits++; > } > return err; > - > -not_jbd: > - jbd_unlock_bh_state(bh); > - __bforget(bh); > - goto drop; > } > > /**
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 9ccef3d6e817..d752d7f6929e 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -1547,7 +1547,7 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) if (!J_EXPECT_JH(jh, !jh->b_committed_data, "inconsistent data on disk")) { err = -EIO; - goto not_jbd; + goto drop; } /* keep track of whether or not this transaction modified us */ @@ -1637,7 +1637,7 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) if (!jh->b_cp_transaction) { JBUFFER_TRACE(jh, "belongs to none transaction"); spin_unlock(&journal->j_list_lock); - goto not_jbd; + goto drop; } /* @@ -1647,7 +1647,7 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) if (!buffer_dirty(bh)) { __jbd2_journal_remove_checkpoint(jh); spin_unlock(&journal->j_list_lock); - goto not_jbd; + goto drop; } /* @@ -1660,10 +1660,9 @@ int jbd2_journal_forget (handle_t *handle, struct buffer_head *bh) __jbd2_journal_file_buffer(jh, transaction, BJ_Forget); spin_unlock(&journal->j_list_lock); } - +drop: jbd_unlock_bh_state(bh); __brelse(bh); -drop: if (drop_reserve) { /* no need to reserve log space for this block -bzzz */ handle->h_buffer_credits++;
jbd2_journal_forget() jumps to 'not_jbd' branch which calls __bforget() in cases where the buffer is clean which is pointless. In case of failed assertion, it can be even argued that it is safer not to touch buffer's dirty bits. Also logically it makes more sense to just jump to 'drop' and that will make logic also simpler when we switch bh_state_lock to a spinlock. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/jbd2/transaction.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)