Red Hat Bugzilla – Bug 656461
cifs: fix problems with filehandle management and reporting of writeback errors
Last modified: 2011-05-19 08:30:16 EDT
CIFS has a really goofy way of dealing with open files on the server. They're tracked via a data structure attached to filp->private_data and those data structures are placed on a list that hangs from the inode. When another process needs an open filehandle on the server, the code will often walk this list and "borrow" one from an existing open file. That's fine, but when it does this, it does no real refcounting. If the filp gets closed, then it waits a "little while" for the refcount to go low and eventually (after 2s) gives up and closes the file anyway. Obviously this is racy and broken, and can lead to data integrity problems. I pushed a patchset upstream to fix many of these problems a month or two ago. This bug is to track backporting that for RHEL6.1.
This request was evaluated by Red Hat Product Management for inclusion in a Red Hat Enterprise Linux maintenance release. Product Management has requested further review of this request by Red Hat Engineering, for potential inclusion in a Red Hat Enterprise Linux Update release for currently deployed products. This request is not yet committed for inclusion in an Update release.
hi Jeff, Could you provide some basic test to cover it? thanks.
Patch(es) available on kernel-2.6.32-112.el6
hi Jeff, I can't find related change in fs/cifs/dir.c for the below patches when I unpack sources and apply patches from <specfile>: http://patchwork.usersys.redhat.com/patch/30035/ http://patchwork.usersys.redhat.com/patch/30037/ http://patchwork.usersys.redhat.com/patch/30044/ http://patchwork.usersys.redhat.com/patch/30050/ http://patchwork.usersys.redhat.com/patch/30051/
(In reply to comment #11) > hi Jeff, > I can't find related change in fs/cifs/dir.c for the below patches when I > unpack sources and apply patches from <specfile>: > http://patchwork.usersys.redhat.com/patch/30035/ > http://patchwork.usersys.redhat.com/patch/30037/ > http://patchwork.usersys.redhat.com/patch/30044/ > http://patchwork.usersys.redhat.com/patch/30050/ > http://patchwork.usersys.redhat.com/patch/30051/ you could login ibm-js22-vios-02-lp2.rhts.eng.bos.redhat.com with root/redhat if you want to check, the build dir is /root/rpmbuild/BUILD/kernel-2.6.32-119.el6/linux-2.6.32-119.el6.ppc64
I did a spot check of the sources on that machine and those patches appear to have been applied. For instance, for: http://patchwork.usersys.redhat.com/patch/30051/ ...the cifs_sb_active and cifs_sb_deactive functions are in cifsfs.c in the source tree. Perhaps if you can clarify what you're trying to check for and how you're performing that check I can try to help more?
(In reply to comment #13) > I did a spot check of the sources on that machine and those patches appear to > have been applied. For instance, for: > > http://patchwork.usersys.redhat.com/patch/30051/ > > ...the cifs_sb_active and cifs_sb_deactive functions are in cifsfs.c in the > source tree. Perhaps if you can clarify what you're trying to check for and how > you're performing that check I can try to help more? hi Jeff, I mean the modification in fs/cifs/dir.c source file, for example, http://patchwork.usersys.redhat.com/patch/30051/ <snip> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c index 2960eb3..9f0536e 100644 --- a/fs/cifs/dir.c +++ b/fs/cifs/dir.c @@ -132,8 +132,7 @@ cifs_bp_rename_retry: struct cifsFileInfo * cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file, - struct vfsmount *mnt, struct tcon_link *tlink, - unsigned int oflags, __u32 oplock) + struct tcon_link *tlink, unsigned int oflags, __u32 oplock) { struct dentry *dentry = file->f_path.dentry; struct cifsFileInfo *pCifsFile; @@ -147,7 +146,6 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file, pCifsFile->pid = current->tgid; pCifsFile->uid = current_fsuid(); pCifsFile->dentry = dget(dentry); - pCifsFile->mnt = mnt; pCifsFile->pfile = file; pCifsFile->invalidHandle = false; pCifsFile->closePend = false; @@ -484,8 +482,7 @@ cifs_create_set_dentry: } pfile_info = cifs_new_fileinfo(newinode, fileHandle, filp, - nd->path.mnt, tlink, oflags, - oplock); + tlink, oflags, oplock); <snip> But I can't find the added line like "struct tcon_link *tlink, unsigned int oflags, __u32 oplock)". Sorry if my expression is unclear.
That's because a later patch moves the entire cifs_new_fileinfo function to file.c. If your intent is to check whether the patches have been applied correctly, then it might make more sense to try and back the patches out in reverse order. Many of these patches alter the same pieces of code in different ways.
(In reply to comment #15) > That's because a later patch moves the entire cifs_new_fileinfo function > to file.c. > > If your intent is to check whether the patches have been applied correctly, > then it might make more sense to try and back the patches out in reverse > order. Many of these patches alter the same pieces of code in different > ways. Thanks Jeff, I've got it. I finish basic function testing also, results passed: https://beaker.engineering.redhat.com/jobs/60763 https://beaker.engineering.redhat.com/jobs/60762 https://beaker.engineering.redhat.com/jobs/60761 https://beaker.engineering.redhat.com/jobs/60735
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHSA-2011-0542.html