[PATCH] F_SETLEASE only on regular files.

[Andrew Bartlett]

On Mon, 2013-06-24 at 07:30 -0400, Jeff Layton wrote:
> On Sun, 23 Jun 2013 10:31:38 +0200
> Andreas Schneider <asn at samba.org> wrote:
> 
> > On Sunday, June 23, 2013 09:38:27 you wrote:
> > > On Sat, Jun 22, 2013 at 10:48:56AM +0200, Andreas Schneider wrote:
> > > > Hello,
> > > > 
> > > > we should set a lease with
> > > > 
> > > >     ret = fcntl(fd, F_SETLEASE, leasetype);
> > > > 
> > > > in source3/smbd/oplock_linux.c only on regular files. Attached is a patch
> > > > to do this. I'm not a VFS expert so the questions is if we shouldn't add
> > > > errno (EINVAL) handling to functions which call SMB_VFS_LINUX_SETLEASE or
> > > > SMB_VFS_NEXT_LINUX_SETLEASE. Maybe at least to not log an error.
> > > 
> > > Question -- what made you look at this? Is there some real
> > > error that you want to fix?
> > 
> > I got a report cause people saw the error in the log file that setting the 
> > lease failed for a pipe.
> > 
> > So if it is not a regular file we should not report and error but just 
> > silently skip it. So it is more about avoiding more of these kind of bug 
> > reports.
> > 
> 
> This patch won't do that though, will it? You'll still return -EINVAL
> here and it'll still log the messages. You're just skipping the
> SETLEASE attempt. Now that I look too, linux_set_kernel_oplock() has
> this:
> 
>         if ( SMB_VFS_LINUX_SETLEASE(fsp, F_WRLCK) == -1) {
>                 DEBUG(3,("linux_set_kernel_oplock: Refused oplock on file %s, "
>                          "fd = %d, file_id = %s. (%s)\n",
>                          fsp_str_dbg(fsp), fsp->fh->fd,
>                          file_id_string_tos(&fsp->file_id),
>                          strerror(errno)));
>                 return False;
>         }
> 
> 
> ...so this is being logged at debug level 3. I think we should just
> close that request as NOTABUG. It seems entirely appropriate to be
> logging these messages at a DEBUG level.

Level 3 certainly seems a reasonable level to log this kind of stuff,
and I certainly don't think adding another system call to the hot path
is the right approach to fixing this. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org