On Mon 17-01-22 18:25:27, Ritesh Harjani wrote: > 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@xxxxxxxxxxxxx> > > > > > > This patch looks good. Feel free to add: > > > > > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > > > > > 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. Yes, probably a good choice. > > 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? Yes, handle is per process so no lock is needed there. > > > 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? Correct. It is the maximum time we had to wait for a new transaction to be created. > 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. All above is correct upto this point. The catch is there can be (and often are) more processes in start_this_handle() waiting in wait_transaction_switching() and then racing to create the new transaction. The process calling jbd2_get_transaction() is not necessarily the one which entered start_this_handle() first and thus t_max_wait would not be really the maximum time someone had to wait. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
