Re: [PATCH] jbd2: do not start commit when t_updates does not back to zero

Re: [PATCH] jbd2: do not start commit when t_updates does not back to zero

[Jan Kara]

On Sun 24-03-19 11:38:35, Liu Song wrote:
> When t_updates back to zero, it guaranteed wake up process which
> waiting on j_wait_updates. If we triggered a commit start without
> considered t_updates, the commit thread wakes up and find t_updates
> is not zero, it have to wait on it once again. So, add checking code
> to avoid this happen.
> 
> Signed-off-by: Liu Song <liu.song11@zte.com.cn>

Do I understand correctly that this is a performance improvement? If yes,
did you measure any benefit of the patch? Because I have some doubts that
t_updates == 0 case is very common.

								Honza

> ---
>  fs/jbd2/transaction.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 79a028a7a579..e0499fd73b1e 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -144,12 +144,13 @@ static void wait_transaction_locked(journal_t *journal)
>  	__releases(journal->j_state_lock)
>  {
>  	DEFINE_WAIT(wait);
> -	int need_to_start;
> +	int need_to_start = 0;
>  	tid_t tid = journal->j_running_transaction->t_tid;
>  
>  	prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
>  			TASK_UNINTERRUPTIBLE);
> -	need_to_start = !tid_geq(journal->j_commit_request, tid);
> +	if (!atomic_read(&journal->j_running_transaction->t_updates))
> +		need_to_start = !tid_geq(journal->j_commit_request, tid);
>  	read_unlock(&journal->j_state_lock);
>  	if (need_to_start)
>  		jbd2_log_start_commit(journal, tid);
> -- 
> 2.19.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: do not start commit when t_updates does not back to zero

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On Mar 29, 2019, at 3:25 PM, Jan Kara <jack@suse.cz> wrote:
> 
> On Sun 24-03-19 11:38:35, Liu Song wrote:
>> When t_updates back to zero, it guaranteed wake up process which
>> waiting on j_wait_updates. If we triggered a commit start without
>> considered t_updates, the commit thread wakes up and find t_updates
>> is not zero, it have to wait on it once again. So, add checking code
>> to avoid this happen.
>> 
>> Signed-off-by: Liu Song <liu.song11@zte.com.cn>
> 
> Do I understand correctly that this is a performance improvement? If yes,
> did you measure any benefit of the patch? Because I have some doubts that
> t_updates == 0 case is very common.

In the past there have been times when periodic journal commits keep a disk
from spinning down, but I don't know if that is the case here...

Cheers, Andreas

>> ---
>> fs/jbd2/transaction.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 79a028a7a579..e0499fd73b1e 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -144,12 +144,13 @@ static void wait_transaction_locked(journal_t *journal)
>> 	__releases(journal->j_state_lock)
>> {
>> 	DEFINE_WAIT(wait);
>> -	int need_to_start;
>> +	int need_to_start = 0;
>> 	tid_t tid = journal->j_running_transaction->t_tid;
>> 
>> 	prepare_to_wait(&journal->j_wait_transaction_locked, &wait,
>> 			TASK_UNINTERRUPTIBLE);
>> -	need_to_start = !tid_geq(journal->j_commit_request, tid);
>> +	if (!atomic_read(&journal->j_running_transaction->t_updates))
>> +		need_to_start = !tid_geq(journal->j_commit_request, tid);
>> 	read_unlock(&journal->j_state_lock);
>> 	if (need_to_start)
>> 		jbd2_log_start_commit(journal, tid);
>> --
>> 2.19.1
>> 
>> 
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] jbd2: do not start commit when t_updates does not back tozero

[Theodore Ts'o]

On Sat, Mar 30, 2019 at 03:24:44PM +0800, liu.song11@zte.com.cn wrote:
> 
> When we unzipped bunch of files under ext4, sometimes got hungtask warning
> which indicate process was waiting on "j_wait_transaction_locked".

Can you give a clean repro of this?  What sort of storage device are
you using, and what sort of file systems and mount options are you
using?  I unzip files under ext4 all the time w/o any problems....

	  	      	    	     - Ted

Re: [PATCH] jbd2: do not start commit when t_updates does not backtozero

[Theodore Ts'o]

On Mon, Apr 01, 2019 at 10:35:04AM +0800, liu.song11@zte.com.cn wrote:
> 
> Our device is CF card(TS8GCF300), mount options are very general(rw,dirsync,
> relatime,data=ordered).
> The hung problem appears under ext4, but the reason is related to the way 
> of use. In our system, there are many RT tasks, which make normal priority 
> tasks survived in harsh environments, such as syslogd. The syslog record is 
> also under the same device, which is really a stumbling block.
> We moved the location of the syslog record to another device and the hungtask 
> problem was solved.

So the general advice which is going to be true for all file systems
is (a) don't try to do any file I/O from real-time tasks, and (b) if
you must do file I/O from a real-time task, be prepared to be willing
to accept your real-time time tasks blocking behind device I/O, thus
destroying your real-time guarantees, and (c) make sure any kernel
threads used by the file system (e.g., such as the jbd2 kernel thread
for ext4) is also given real-time priority.

Was syslogd being run with real-time priority?  If not, you're going
to not really have real-time performance unles you make sure syslog(3)
calls don't block waiting for syslogd to acknowledge the write.  See
syslog-async as referenced here[1].

[1] https://stackoverflow.com/questions/208098/can-syslog-performance-be-improved

What I suspect was happening was you were using standard syslog(3)
which was blocking for syslogd to respond, syslog was by default
trying to fsync every single log entry before returning success (this
can changed by making the appropriate change to syslog.conf; that's a
different change suggested by [1] above), and so your real-time task
that was calling syslog was blocking.  Since it was a real-time task,
and the jbd2 kernel thread was not a real time thread, this caused a
deadlock.

There are multiple things you can try to optimize (and with real-time
systems, getting configuration right is really, REALLY, critical), but
it sounds like the real root cause is you have a real-time task using
syslog(3).  Don't do that.  It will probably cause you problems in
multiple dimensions.

						- Ted

P.S.  Especially don't try using syslog in real-time tasks if said
real-time system is going to be used in commercial aviation.  It might
cause scandals ala the 737 MAX.  :-)