From: Ritesh Harjani <riteshh@linux.ibm.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, Jan Kara <jack@suse.com>,
Andreas Dilger <adilger.kernel@dilger.ca>,
tytso@mit.edu, Eric Whitney <enwlinux@gmail.com>
Subject: Re: [PATCH 6/6] jbd2: No need to use t_handle_lock in jbd2_journal_wait_updates
Date: Mon, 17 Jan 2022 18:25:27 +0530 [thread overview]
Message-ID: <20220117125527.ienv3drg5whiryrr@riteshh-domain> (raw)
In-Reply-To: <20220113123842.3rpfcyecylt5n3wo@riteshh-domain>
On 22/01/13 06:08PM, Ritesh Harjani wrote:
> On 22/01/13 12:27PM, Jan Kara wrote:
> > On Thu 13-01-22 08:56:29, Ritesh Harjani wrote:
> > > Since jbd2_journal_wait_updates() uses waitq based on t_updates atomic_t
> > > variable. So from code review it looks like we don't need to use
> > > t_handle_lock spinlock for checking t_updates value.
> > > Hence this patch gets rid of the spinlock protection in
> > > jbd2_journal_wait_updates()
> > >
> > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> >
> > This patch looks good. Feel free to add:
> >
> > Reviewed-by: Jan Kara <jack@suse.cz>
> >
> > Actually looking at it, t_handle_lock seems to be very much unused. I agree
Thanks Jan for your help in this.
I have dropped this patch from v2 in order to discuss few more things and I felt
killing t_handle_lock completely can be sent in a seperate patch series.
>
> I too had this thought in mind. Thanks for taking a deeper look into it :)
>
> >
> > we don't need it when waiting for outstanding handles but the only
> > remaining uses are:
> >
> > 1) jbd2_journal_extend() where it is not needed either - we use
> > atomic_add_return() to manipulate t_outstanding_credits and hold
> > j_state_lock for reading which provides us enough exclusion.
I looked into jbd2_journal_extend and yes, we don't need t_handle_lock
for updating transaction->t_outstanding_credits, since it already happens with
atomic API calls.
Now I do see we update handle->h_**_credits in that function.
But I think this is per process (based on task_struct, current->journal_info)
and doesn't need a lock protection right?
> >
> > 2) update_t_max_wait() - this is the only valid use of t_handle_lock but we
> > can just switch it to cmpxchg loop with a bit of care. Something like:
> >
> > unsigned long old;
> >
> > ts = jbd2_time_diff(ts, transaction->t_start);
> > old = transaction->t_max_wait;
> > while (old < ts)
> > old = cmpxchg(&transaction->t_max_wait, old, ts);
I think there might be a simpler and more straight forward way for updating
t_max_wait.
I did look into the t_max_wait logic and where all we are updating it.
t_max_wait is the max wait time in starting (&attaching) a _new_ running
transaction by a handle. Is this understaning correct?
From code I don't see t_max_wait getting updated for the time taken in order
to start the handle by a existing running transaction.
Here is how -
update_t_max_wait() will only update t_max_wait if the
transaction->t_start is after ts
(ts is nothing but when start_this_handle() was called).
1. This means that for transaction->t_start to be greater than ts, it has to be
the new transaction that gets started right (in start_this_handle() func)?
2. Second place where transaction->t_start is updated is just after the start of
commit phase 7. But this only means that this transaction has become the
commit transaction. That means someone has to alloc a new running transaction
which again is case-1.
Now I think this spinlock was added since multiple processes can start a handle
in parallel and attach a running transaction.
Also this was then moved within CONFIG_JBD2_DEBUG since to avoid spinlock
contention on a SMP system in starting multiple handles by different processes.
Now looking at all of above, I think we can move update_t_max_wait()
inside jbd2_get_transaction() in start_this_handle(). Because that is where
a new transaction will be started and transaction->t_start will be greater then
ts. This also is protected within j_state_lock write_lock, so we don't need
spinlock.
This would also mean that we can move t_max_wait outside of CONFIG_JBD2_DEBUG
and jbd2_journal_enable_debug.
Jan, could you confirm if above understaning is correct and shall I go ahead
with above changes?
-ritesh
> >
> > So perhaps you can add two more patches to remove other t_handle_lock uses
> > and drop it completely.
>
> Thanks for providing the details Jan :)
> Agree with jbd2_journal_extend().
> I did looked a bit around t_max_wait and
> I agree that something like above could work. I will spend some more time around
> that code and will submit those changes together in v2.
>
> -ritesh
>
> >
> > Honza
> >
> > > ---
> > > include/linux/jbd2.h | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > > index 34b051aa9009..9bef47622b9d 100644
> > > --- a/include/linux/jbd2.h
> > > +++ b/include/linux/jbd2.h
> > > @@ -1768,22 +1768,18 @@ static inline void jbd2_journal_wait_updates(journal_t *journal)
> > > if (!commit_transaction)
> > > return;
> > >
> > > - spin_lock(&commit_transaction->t_handle_lock);
> > > while (atomic_read(&commit_transaction->t_updates)) {
> > > DEFINE_WAIT(wait);
> > >
> > > prepare_to_wait(&journal->j_wait_updates, &wait,
> > > TASK_UNINTERRUPTIBLE);
> > > if (atomic_read(&commit_transaction->t_updates)) {
> > > - spin_unlock(&commit_transaction->t_handle_lock);
> > > write_unlock(&journal->j_state_lock);
> > > schedule();
> > > write_lock(&journal->j_state_lock);
> > > - spin_lock(&commit_transaction->t_handle_lock);
> > > }
> > > finish_wait(&journal->j_wait_updates, &wait);
> > > }
> > > - spin_unlock(&commit_transaction->t_handle_lock);
> > > }
> > >
> > > /*
> > > --
> > > 2.31.1
> > >
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
next prev parent reply other threads:[~2022-01-17 12:55 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-13 3:26 [PATCH 0/6] ext4/jbd2: inline_data fixes and some cleanups Ritesh Harjani
2022-01-13 3:26 ` [PATCH 1/6] ext4: Fix error handling in ext4_restore_inline_data() Ritesh Harjani
2022-01-13 10:58 ` Jan Kara
2022-01-13 3:26 ` [PATCH 2/6] ext4: Remove redundant max inline_size check in ext4_da_write_inline_data_begin() Ritesh Harjani
2022-01-13 10:58 ` Jan Kara
2022-01-13 3:26 ` [PATCH 3/6] ext4: Fix error handling in ext4_fc_record_modified_inode() Ritesh Harjani
2022-01-13 11:00 ` Jan Kara
2022-01-13 3:26 ` [PATCH 4/6] jbd2: Cleanup unused functions declarations from jbd2.h Ritesh Harjani
2022-01-13 11:01 ` Jan Kara
2022-01-13 3:26 ` [PATCH 5/6] jbd2: Refactor wait logic for transaction updates into a common function Ritesh Harjani
2022-01-13 11:30 ` Jan Kara
2022-01-13 12:17 ` Ritesh Harjani
2022-01-13 3:26 ` [PATCH 6/6] jbd2: No need to use t_handle_lock in jbd2_journal_wait_updates Ritesh Harjani
2022-01-13 11:27 ` Jan Kara
2022-01-13 12:38 ` Ritesh Harjani
2022-01-17 12:55 ` Ritesh Harjani [this message]
2022-01-17 14:38 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220117125527.ienv3drg5whiryrr@riteshh-domain \
--to=riteshh@linux.ibm.com \
--cc=adilger.kernel@dilger.ca \
--cc=enwlinux@gmail.com \
--cc=jack@suse.com \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).