On Wed, 23 Mar 2011 12:32:03 -0500 Steve French <[email protected]> wrote:
> On Wed, Mar 23, 2011 at 11:14 AM, Jeff Layton <[email protected]> wrote: > > On Wed, 23 Mar 2011 12:27:31 +0300 > > Pavel Shilovsky <[email protected]> wrote: > > > >> 2011/3/23 Steve French <[email protected]>: > >> > 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. > No. RHEL5 at that time didn't have a launder page method (it does now, but that's another story). -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
