Re: [JBD] change batching logic to improve O_SYNC performance

[Andreas Dilger <adilger@clusterfs.com>]

On Dec 15, 2005  15:55 -0800, Andrew Morton wrote:
> Yeah, 2.4 has yield().  The O(1) yield semantics resulted in a performance
> catastrophe in ext3 when the system was busy, so the batching code got
> changed to a one-jiffy-sleep.  I don't think we can go back to yield().
> 
> Worst-case we should just dump the batching code: single-threaded
> O_SYNC/fsync is probably a commoner case than multi-threaded, dunno.

I hope we don't discard the sync batching entirely.  This is commonly used
by knfsd and also Lustre to aggregate sync IO operations from many clients.
Having to wait for a whole transaction commit for every RPC would suck badly.

> How's about something simple like just saying "if the last process which
> did a synchronous write is not this process, do the batching thing".
> 
> 
> Something like this?

Looks interesting at least.

> diff -puN fs/jbd/transaction.c~jbd-fix-transaction-batching fs/jbd/transaction.c
> --- 25/fs/jbd/transaction.c~jbd-fix-transaction-batching	Thu Dec 15 15:47:52 2005
> +++ 25-akpm/fs/jbd/transaction.c	Thu Dec 15 15:53:28 2005
> @@ -1308,6 +1308,7 @@ int journal_stop(handle_t *handle)
>  	transaction_t *transaction = handle->h_transaction;
>  	journal_t *journal = transaction->t_journal;
>  	int old_handle_count, err;
> +	pid_t pid;
>  
>  	J_ASSERT(transaction->t_updates > 0);
>  	J_ASSERT(journal_current_handle() == handle);
> @@ -1333,8 +1334,15 @@ int journal_stop(handle_t *handle)
>  	 * It doesn't cost much - we're about to run a commit and sleep
>  	 * on IO anyway.  Speeds up many-threaded, many-dir operations
>  	 * by 30x or more...
> +	 *
> +	 * But don't do this if this process was the most recent one to
> +	 * perform a synchronous write.  We do this to detect the case where a
> +	 * single process is doing a stream of sync writes.  No point in waiting
> +	 * for joiners in that case.
>  	 */
> -	if (handle->h_sync) {
> +	pid = current->pid;
> +	if (handle->h_sync && journal->j_last_sync_writer != pid) {
> +		journal->j_last_sync_writer = pid;
>  		do {
>  			old_handle_count = transaction->t_handle_count;
>  			schedule_timeout_uninterruptible(1);
> diff -puN include/linux/jbd.h~jbd-fix-transaction-batching include/linux/jbd.h
> --- 25/include/linux/jbd.h~jbd-fix-transaction-batching	Thu Dec 15 15:48:25 2005
> +++ 25-akpm/include/linux/jbd.h	Thu Dec 15 15:53:46 2005
> @@ -23,6 +23,7 @@
>  #define jfs_debug jbd_debug
>  #else
>  
> +#include <linux/types.h>
>  #include <linux/buffer_head.h>
>  #include <linux/journal-head.h>
>  #include <linux/stddef.h>
> @@ -618,6 +619,7 @@ struct transaction_s 
>   * @j_wbuf: array of buffer_heads for journal_commit_transaction
>   * @j_wbufsize: maximum number of buffer_heads allowed in j_wbuf, the
>   *	number that will fit in j_blocksize
> + * @j_last_sync_writer: most recent pid which did a synchronous write
>   * @j_private: An opaque pointer to fs-private information.
>   */
>  
> @@ -807,6 +809,8 @@ struct journal_s
>  	struct buffer_head	**j_wbuf;
>  	int			j_wbufsize;
>  
> +	pid_t			j_last_sync_writer;
> +
>  	/*
>  	 * An opaque pointer to fs-private information.  ext3 puts its
>  	 * superblock pointer here
> _
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.