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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
