[PVFS-developers] IOD File Descriptor Leak

[Rob Ross]

Oops, you're right...good catch.  Hmm.  Of course the problem is that we 
don't *have* a socket to attach them to other than the mgr's socket at the 
time the file is opened.

So those aren't really *lost* per se; sooner or later the mgr will tell 
the iod that it doesn't need to hold on to any more open file structures 
for a given file.

At least, this should happen.  If it isn't, we should figure out why.

However, I don't like my patch very much any more :(.  I think your idea 
of reassigning at do_rw_req() time is reasonable.

The capability number is simply an integer that is handed to a client at 
open time to differentiate one open instance from another.  It's zero for 
the first instance of an open file, so that's probably why you were seeing 
zero all the time.

I'm now have the code reassign in do_rw_req() to the new socket.  Could
you try out the CVS version?

Rob

On Tue, 16 Sep 2003, Porter Don wrote:

> Thanks Rob.  This is very helpful.
> 
> The problem with initializing the sock field in do_open_req() is that (at
> least in some observation I did), this seems to always be called by the
> manager.  
> 
> I looked at pvfs_open.c and it seems to call the manager with a MGR_OPEN
> request, but it sends a NOOP to the iods just to initialize the connection.
> In mgr.c:do_open() the manager sends the IOD_OPEN request to the iods if
> this is the first time the file has been opened.  
> 
> In some tests I did with a similar one-line fix, all finfo's got associated
> with the manager (usually sock fd 0).  So unless they are re-assigned in
> do_rw_req(), they will still not be cleaned up unless the manager is
> bounced.
> 
> Also, what is the capability number?  I can't figure that out.  My wrong
> conclusion that it grabbed the first finfo was based on the fact that all
> requests I see used a capability number of 0.
> 
> Thanks,
> Don
> 
> -----Original Message-----
> From: Rob Ross
> To: Porter Don
> Cc: 'pvfs-developers@www.beowulf-underground.org'
> Sent: 9/16/03 11:28 AM
> Subject: Re: [PVFS-developers] IOD File Descriptor Leak
> 
> Hi Don,
> 
> I see what you're talking about; the file structure is allocated in 
> iod.c:do_open_req() but the sock field isn't set until
> jobs.c:do_rw_req().
> 
> This is a problem because the flist_purge_sock() call then cannot 
> correctly remove the file structure on socket close.
> 
> However, your assertion that do_rw_job() just grabs the first finfo
> isn't 
> correct.  That search is based on both the ino and the capability
> number, 
> which is different for every client.
> 
> I'm not sure that there is still a clear reason to have multiple file 
> descriptors for each socket; however, it is currently partially used for
> 
> permission purposes, and it isn't clear to me how moving to a single FD 
> would change how the kernel perceives what the iod is doing.
> 
> I'm checking in a single-line fix that associated the socket with the
> file 
> structure at open time.  I think that this will fix the leak.  If you
> want 
> to investigate changing to using a single FD for each file, that's cool,
> 
> but I'm a little hesitant to switch that up without significant testing.
> 
> Thanks for the leak catch!
> 
> Regards,
> 
> Rob
> 
> On Tue, 16 Sep 2003, Porter Don wrote:
> 
> > I have been noticing on some of the iods what appears to be a file
> > descriptor leak.  In looking at the iod code, it seems to be happening
> when
> > a job cancels between opening a file and doing a rw job.  The socket
> number
> > in the finfo struct is initialized to -1 and doesn't get set to a real
> > socket until a rw job gets it from the list.  If this field never gets
> > initialized, the finfo will not be cleaned up when the socket closes.
> The
> > leak seems to be introduced because right now the do_rw_job function
> just
> > grabs the first finfo for the inode it wants and assigns its socket if
> it is
> > uninitialized, completely ignoring all but the first finfo on the
> linked
> > list.
> > 
> > This raises the question, is there any reason to have multiple file
> > descriptors for the same inode file at all?  The iods are single
> threaded
> > and are effectively sharing one anyway.  It seems that perhaps
> changing the
> > finfo structure to keep a linked list of sockets as a sort of
> "reference
> > count" (like the mgr) might be more efficient and plug up this leak.
> I
> > suppose this would also need to be coupled with having something in
> > check_socks that would clean up unclaimed finfo structs after a period
> of
> > idleness.
> > 
> > Anyway, I am about to start working on this, but I thought I would
> bounce it
> > off of the list and see if anyone had any insights.
> 
>