xfs
[Top] [All Lists]

Re: [Patch] xfs: serialise unaligned direct IOs

To: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
Subject: Re: [Patch] xfs: serialise unaligned direct IOs
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 3 Nov 2011 03:02:46 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <CADDb1s0WUfvt8N+hMATboKxbMUZdk2N-R2e=KFH2JvGUjbigBg@xxxxxxxxxxxxxx>
References: <CADDb1s0WUfvt8N+hMATboKxbMUZdk2N-R2e=KFH2JvGUjbigBg@xxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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.

<Prev in Thread] Current Thread [Next in Thread>