Bug 763196 (GLUSTER-1464)

Summary: fd leak after rename
Product: [Community] GlusterFS Reporter: Krishna Srinivas <krishna>
Component: nfsAssignee: Shehjar Tikoo <shehjart>
Status: CLOSED CURRENTRELEASE QA Contact:
Severity: medium Docs Contact:
Priority: low    
Version: nfs-alphaCC: gluster-bugs, vijay
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: Type: ---
Regression: RTP Mount Type: nfs
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Krishna Srinivas 2010-08-27 16:52:50 EDT
Shehjar this bug affects vmstor ... so let me know asap if this fix is harmful.
Comment 1 Krishna Srinivas 2010-08-27 19:45:23 EDT
echo junk > test2
touch test1
mv test1 test2

this will cause fd leak:

[root@brick1 testing]# ls -l /proc/30418/fd
lrwx------ 1 root root 64 Aug 26 18:33 20 -> /export/export/testing/test2 (deleted)


This fixed it: (but not sure if this is the "right" way of doing things):

int32_t
nfs3svc_rename_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                    int32_t op_ret, int32_t op_errno, struct iatt *buf,
                    struct iatt *preoldparent, struct iatt *postoldparent,
                    struct iatt *prenewparent, struct iatt *postnewparent)
{
        int                     ret = -EFAULT;
        nfsstat3                stat = NFS3ERR_SERVERFAULT;
        nfs3_call_state_t       *cs = NULL;
        fd_t                    *openfd = NULL;
        struct nfs3_state       *nfs3 = NULL;

        cs = frame->local;
        if (op_ret == -1) {
                stat = nfs3_errno_to_nfsstat3 (op_errno);
                goto nfs3err;
        }

        stat = NFS3_OK;
/* Kris: bug fix for rename fd leak */
        /* Close any cached fds so that when any currently active write                                                                                      
         * finishes, the file is finally removed.                                                                                                            
         */
         openfd = fd_lookup (cs->resolvedloc.inode, 0);
         nfs3 = rpcsvc_request_program_private (cs->req);
         if (openfd) {
                fd_unref (openfd);
                nfs3_fdcache_remove (nfs3, openfd);
         }
         /* do inode_unref(cs->resolvedloc.inode) if needed */
/* Kris: bug fix for rename fd leak */
nfs3err:
        nfs3_log_common_res (rpcsvc_request_xid (cs->req), "RENAME", stat,-ret);
Comment 2 Shehjar Tikoo 2010-08-28 01:09:24 EDT
Your change seems about right if it fixes it, go ahead and use it, i'll make the same change here and take it through some tests to be sure
Comment 3 Shehjar Tikoo 2010-08-28 01:48:43 EDT
the inode_unref  on dst is needed, but (a). i am not sure if nfs is the right place for it, need to look into inode.c code as well, (b) need to think about the exact if {} condition i need to use to unref in rename_cbk because the inode is resolvedloc will also be unref in nfs3_call_state_wipe causing an assertion failure there if the inode has been unrefd previously.
Comment 4 Vijay Bellur 2010-08-31 07:44:53 EDT
PATCH: http://patches.gluster.com/patch/4425 in master (nfs3: Close dst cached fd & unref inode on rename)
Comment 5 Shehjar Tikoo 2010-08-31 22:47:30 EDT
Regression Test:

1. Export a simple posix through gnfs.

2. Mount as:

mount <server>:/posix /mnt

3. Run the commands:

$ echo "asdasdasdads" > /mnt/test2
$ touch /mnt/test1
$ mv /mnt/test1 /mnt/test2

4. Then inspect the nfs server's entry in /proc using the command:

$ ls -l /proc/<GNFS-PID>/fd
lrwx------ 1 root root 64 Aug 26 18:33 20 -> /posixexportdir/test2

If the output includes an entry for test2, there is a regression.
Comment 6 Vijay Bellur 2010-09-02 05:17:22 EDT
PATCH: http://patches.gluster.com/patch/4478 in master (nfs3: Do not unref dst inode on rename cbk)