On Wed, Mar 23, 2011 at 11:14 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 23 Mar 2011 12:27:31 +0300
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> 2011/3/23 Steve French <smfrench@xxxxxxxxx>:
>> > Pavel,
>> > Jeff and I talked about invalidate_mapping and its uses this
>> > afternoon. A few comments:
>> > 1) Take a look at what happens on filemap_fdatawait call in
>> > invalidate_mapping to see if it makes sense to return errors through
>> > back to some of the callers of invalidate_mapping, in particular,
>> > strict_fsync. If we can't write out the file data (ENOSPC, or host
>> > down etc.), we want to make sure that the return code gets sent back
>> > on any calls that can reasonably expect such an error.
>>
>> vfs_fsync_range has already done filemap_fdatawait call - so, in this
>> case there is no need to do it again in cifs_invalidate_mapping. The
>> only reason for calling this is to invalidate_inode_pages2 but any
>> error there shouldn't affect fsync behavior, as I think.
>>
>> > 2) If invalidate_inode_pages2 fails (e.g. with EBUSY, because one of
>> > the pages couldn't be freed from the mapping because it just got
>> > redirtied right after we flushed it the line before) we set the
>> > mapping to invalid but don't check
>> > cifs_i->invalid_mapping
>> > in many places. Should we add checks for cifs_i->invalid_mapping in
>> > more places?
>>
>> The one place where we should add such a check is read call, but it
>> needs cifs_revalidate_file instead (that I am going to provide next)
>> before generic_file_aio_read. In this case cifs_revalidate will check
>> for invalid_mapping and needs to return a error if we could not
>> invalidate all inode pages in cifs_invalidate_mapping (because it
>> returns wrong data to the read call). But as you noticed lseek
>> shouldn't think about this error and it uses cifs_revalidate_file too.
>>
>> So, we may add extra check for -EBUSY error code in callers of
>> cifs_revalidate_{dentry,file} and cifs_invaliadate_mapping and
>> separate them into two groups:
>> 1) that aware about -EBUSY error code and return a error the it's caller.
>> these are: cifs_d_revalidate, cifs_file_aio_read (future
>> implementation), cifs_file_strict_mmap.
>> 2) that doesn't aware about it and return ok in this case.
>> these are: cifs_getattrs, cifs_lseek, cifs_fsync.
>>
>> Thoughts?
>>
>
> The main reason I think we need to reconsider that error is that I
> spent several months tracking down a rather nasty data corruption bug
> relating to mmap on NFS in RHEL5 a few years ago:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=435291
>
> Part of the problem there was that the NFS client ignored the return
> code from invalidate_inode_pages2. The other part of the problem was a
> lack of synchronization between mmap calls and the page fault handler.
>
> Needless to say, this was not a fun problem to track down. I think you
> need to be very careful about ignoring errors from
> invalidate_inode_pages2 and think carefully about what a failure there
> means for all cases.
I agree - need to be careful, but IIRC the NFS problem would be that
it has a launder_page method which is returning an error through
invalidate_inode_pages2 while in the cifs case the data is forced to
be written out through filemap_fdatawrite or filemap_write_and_wait.
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
