Re: [Patch] xfs: serialise unaligned direct IOs

[Christoph Hellwig <hch@infradead.org>]

On Thu, Nov 03, 2011 at 11:37:03AM +0530, Amit Sahrawat wrote:
> This is needed for long term kernel 2.6.35.14.
> Please let me know for any changes/suggestions.

It sounds like a fine candidate to backport, although the context
differs a lot from the actual changes commited to mainline.  A few
comments below:

> they are overlapping IO and the result of concurrent overlapping IOs
> is undefined - the result of either IO is a valid result so we let
> them race. Hence we only penalise unaligned IO, which already has a
> major overhead compared to aligned IO so this isn't a major problem.
> 

You probably should keep the original Signoff and reviewed-by tags,
and add your editor note on the top into [ ]  brackets.

> +		if (!need_i_mutex && ( unaligned_io || mapping->nrpages || pos >
> ip->i_size)) {

no space after the opening brace please, and split overly-long lines
into two:

		if (!need_i_mutex &&
		    (unaligned_io || mapping->nrpages || pos > ip->i_size)) {

>  		if (need_i_mutex) {
> -			/* demote the lock now the cached pages are gone */
> -			xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);

> +			if (unaligned_io)
> +	                       	xfs_ioend_wait(ip);
> +	               /* demote the lock now the cached pages are gone if we can */
> +        	        else {
> +                 	        xfs_ilock_demote(ip, XFS_IOLOCK_EXCL);
> +	                        iolock = XFS_IOLOCK_SHARED;
> +        	        }

Please use the comment that was used upstream here:

			/*
			 * If we are doing unaligned IO, wait for all other IO
			 * to drain, otherwise demote the lock if we had to
			 * flush cached pages.
			 */

>  			mutex_unlock(&inode->i_mutex);
> 
> -			iolock = XFS_IOLOCK_SHARED;


>  			need_i_mutex = 0;

You also need to make the i_mutex unlock and need_i_mutex update
conditional here, otherwise you still serialize all O_DIRECT writes.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs