Re: [PATCH 4/5] cifs: take reference to inode for oplock breaks

On Fri, 21 Aug 2009 14:08:24 -0500
Steve French <[email protected]> wrote:

> On Fri, Aug 21, 2009 at 2:01 PM, Steve French <[email protected]> wrote:
> 
> >
> >
> > On Fri, Aug 21, 2009 at 10:48 AM, Jeff Layton <[email protected]> wrote:
> >
> >>
> >> As long as you can do so in a non-racy way, then I'm not opposed to
> >> that long-term. The problem though is that I don't have a lot of
> >> confidence in the open file tracking code. It's extremely hard to
> >> follow and definitely has races. I don't think it's really possible to
> >> do what you suggest safely until the open file tracking code has been
> >> fixed.
> >>
> >> For now, I'm pretty sure this set should fix the problems that users
> >> are hitting in the oplock codepath today. I'd like to fix that first
> >> before we embark on a redesign of it.
> >>
> >
> > You may be right that this should be two stages, but your 2nd and 3rd patch
> > are already large, so I doubt that it would grow (and might make it easier
> > to follow and more logical).
> >
> 
> There is one other thing to consider (which might lean toward your inode
> based approach rather than what I suggested about tie of the
> oplock to the file struct for the fid) ... we need to move to 1st attempting
> "batch oplocks" (as was discussed in June) to allow safer caching
> across close (and would also helps with multiple opens from the same
> client) - this will necessitate making the routine which frees inodes
> aware of oplocks.   In the short run I was planning on trying this approach
> (grab one batch oplock on open, and only rely on the other type of oplocks
> when we can't get or lose the batch oplock) with the smb2 code and
> after a few months if it works ok consider fixing the cifs code.  In the
> meantime if we uses patches 2 through 4 as is (one and five are
> obviously fine), it would be great if we could get at least one more ack.
> I haven't found any problems yet but they are big.
> 
> 

Sorry about the size, but there were a number of races and potential
use-after-free holes in that code. I tried to do the set so that it
made a logical progression of changes, but the patches do touch the
same code numerous times. If it would be more helpful, I can send you
the set as a single large patch.

Thanks again for reviewing it promptly.

-- 
Jeff Layton <[email protected]>