[Patch] xfs: serialise unaligned direct IOs

[Amit Sahrawat]

Thanks Chrisoph for the suggestions.

On Thu, Nov 3, 2011 at 12:32 PM, Christoph Hellwig <hch at infradead.org> 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