xfs
[Top] [All Lists]

Re: [Patch] xfs: serialise unaligned direct IOs

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [Patch] xfs: serialise unaligned direct IOs
From: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
Date: Thu, 3 Nov 2011 15:29:18 +0530
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=jj7NAS5gyURGpD3/Xooa8doRRkKaUDMIFGTzN2N+Qpk=; b=rRv7A40SV0Cy+s/2IGxoHam5l0spJ/1+x+/wp3md1wGUHsqU1kKx8xj5NCnSZrNlmh leznTEeIBqNd5l4aOcVpCz9STXfN5xeE8ZIZAEcx2lYAml0OPwOBsgvlIXPQ8MSMV1m5 Jh6E031BIOJBvJMEEDFmaL0VPxB3WZlUhGmD4=
In-reply-to: <20111103070246.GA10579@xxxxxxxxxxxxx>
References: <CADDb1s0WUfvt8N+hMATboKxbMUZdk2N-R2e=KFH2JvGUjbigBg@xxxxxxxxxxxxxx> <20111103070246.GA10579@xxxxxxxxxxxxx>
Thanks Chrisoph for the suggestions.

On Thu, Nov 3, 2011 at 12:32 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 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.
Ok, will do so in the final patch. Actually was unaware of information
to keep in backported patches?
>
>> +             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:
Probably running checkpatch.pl will help.
>
>                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.
>
you mean, keeping need_i_mutex=0 and mutex_unlock as part of 'else' statement.


Thanks & Regards,
Amit Sahrawat

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