fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Beschorner Daniel]

>>> static void lease_release_private_callback(struct file_lock *fl) 
>>> { 
>>>         if (!fl->fl_file) 
>>>                 return; 
>>>         f_delown(fl->fl_file); 
>>> =>      fl->fl_file->f_owner.signum = 0; 
>>> } 

>> Why should the lease cleanup code be resetting f_owner.signum? That 
>> looks wrong. 
>> Stephen, I think this line of code predates the CITI changes. Do you 
>> know who added it and why? 

>Because when the original code was written, it was only called when we
got 
>a fcntl(F_SETLEASE, F_UNLCK) call.  The code got moved incorrectly and 
>noone noticed.

Does somebody have a patch for this issue? It breaks one important
application (Samba) in its default configuration.

Daniel

Re: fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On Tue, 2006-08-08 at 07:38 +0200, Beschorner Daniel wrote:
> >>> static void lease_release_private_callback(struct file_lock *fl) 
> >>> { 
> >>>         if (!fl->fl_file) 
> >>>                 return; 
> >>>         f_delown(fl->fl_file); 
> >>> =>      fl->fl_file->f_owner.signum = 0; 
> >>> } 
> 
> >> Why should the lease cleanup code be resetting f_owner.signum? That 
> >> looks wrong. 
> >> Stephen, I think this line of code predates the CITI changes. Do you 
> >> know who added it and why? 
> 
> >Because when the original code was written, it was only called when we
> got 
> >a fcntl(F_SETLEASE, F_UNLCK) call.  The code got moved incorrectly and 
> >noone noticed.
> 
> Does somebody have a patch for this issue? It breaks one important
> application (Samba) in its default configuration.
> 
> Daniel

I believe this ought to fix it.

Cheers,
  Trond


[-- Attachment #2: linux-2.6.18-fix_lease_signals.dif --]
[-- Type: message/rfc822, Size: 1243 bytes --]

From: Trond Myklebust <Trond.Myklebust@netapp.com>
Subject: No Subject
Date: Tue, 08 Aug 2006 09:26:30 -0400
Message-ID: <1155043590.5673.14.camel@localhost>

fcntl(F_SETSIG) no longer works on leases because
lease_release_private_callback() gets called as the lease is copied in
order to initialise it.
The problem is that lease_alloc() performs an unnecessary initialisation,
which sets the lease_manager_ops. Avoid the problem by allocating the
target lease structure using locks_alloc_lock().

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/locks.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index b0b41a6..d7c5339 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1421,8 +1421,9 @@ static int __setlease(struct file *filp,
 	if (!leases_enable)
 		goto out;
 
-	error = lease_alloc(filp, arg, &fl);
-	if (error)
+	error = -ENOMEM;
+	fl = locks_alloc_lock();
+	if (fl == NULL)
 		goto out;
 
 	locks_copy_lock(fl, lease);
@@ -1430,6 +1431,7 @@ static int __setlease(struct file *filp,
 	locks_insert_lock(before, fl);
 
 	*flp = fl;
+	error = 0;
 out:
 	return error;
 }

Re: fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Beschorner Daniel]

> I believe this ought to fix it.
>
> Cheers,
>  Trond

Thanks, the Samba problems are gone with this patch.
Maybe a 2.6.17.x candidate?

Daniel

Re: fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Chuck Ebbert]

In-Reply-To: <eabdhq$nca$1@sea.gmane.org>

On Thu, 27 Jul 2006 16:08:53 -0600, Orion Poplawski wrote:
>
> fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.
>
> The attached program (oplocktest.c) illustrates.

I added some debug statements to your code:

=>      printf("before setlease: signal number = %d\n", fcntl(fd, F_GETSIG));
        ret = fcntl(fd, F_SETLEASE, leasetype);
        if (ret == -1 && errno == EACCES) {
                set_capability(CAP_LEASE);
                ret = fcntl(fd, F_SETLEASE, leasetype);
        }
=>      printf("after setlease: signal number = %d\n", fcntl(fd, F_GETSIG));

And I get:

before setlease: signal number = 34
after setlease: signal number = 0

So the fcntl(F_SETLEASE) is resetting the signal number.  I don't think
it's supposed to do that.

That seems to be caused by:

| From: Trond Myklebust <Trond.Myklebust@netapp.com>
| Date: Mon, 20 Mar 2006 18:44:05 +0000 (-0500)
| Subject: VFS: Fix __posix_lock_file() copy of private lock area
| X-Git-Tag: v2.6.17-rc1
| X-Git-Url: http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47831
|
| VFS: Fix __posix_lock_file() copy of private lock area
|
| The struct file_lock->fl_u area must be copied using the fl_copy_lock()
| operation.

In this change:

|  */
| void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
| {
|+       locks_release_private(new);
|+
|        new->fl_owner = fl->fl_owner;
|        new->fl_pid = fl->fl_pid;
|        new->fl_file = fl->fl_file;

Which ends up calling this:

static void lease_release_private_callback(struct file_lock *fl)
{
        if (!fl->fl_file)
                return;

        f_delown(fl->fl_file);
=>      fl->fl_file->f_owner.signum = 0;
}

I'm not sure how to fix it, though (if that's really the problem, but I
think it is.)

-- 
Chuck

Re: fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Trond Myklebust]

On Mon, 2006-07-31 at 02:21 -0400, Chuck Ebbert wrote:

> static void lease_release_private_callback(struct file_lock *fl)
> {
>         if (!fl->fl_file)
>                 return;
> 
>         f_delown(fl->fl_file);
> =>      fl->fl_file->f_owner.signum = 0;
> }

Why should the lease cleanup code be resetting f_owner.signum? That
looks wrong.

Stephen, I think this line of code predates the CITI changes. Do you
know who added it and why?

Cheers,
  Trond

Re: fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Stephen Rothwell]

On Mon, 31 Jul 2006 11:23:52 -0700 Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
>
> On Mon, 2006-07-31 at 02:21 -0400, Chuck Ebbert wrote:
> 
> > static void lease_release_private_callback(struct file_lock *fl)
> > {
> >         if (!fl->fl_file)
> >                 return;
> > 
> >         f_delown(fl->fl_file);
> > =>      fl->fl_file->f_owner.signum = 0;
> > }
> 
> Why should the lease cleanup code be resetting f_owner.signum? That
> looks wrong.
> 
> Stephen, I think this line of code predates the CITI changes. Do you
> know who added it and why?

Because when the original code was written, it was only called when we got
a fcntl(F_SETLEASE, F_UNLCK) call.  The code got moved incorrectly and
noone noticed.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

[Orion Poplawski]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

fctnl(F_SETSIG) no longer works in 2.6.17, does in 2.6.16.

The attached program (oplocktest.c) illustrates.  Compile and run with 
strace.  In another
shell do "echo >> oplockstest.c".  On 2.6.16 we get:

fcntl64(3, F_SETSIG, 0x23)              = 0
fcntl64(3, 0x400 /* F_??? */, 0x1)      = 0
nanosleep({50, 0}, 0)                   = ? ERESTART_RESTARTBLOCK (To be 
restarted)
--- SIGRT_3 (Real-time signal 1) @ 0 (0) ---
+++ killed by SIGRT_3 +++

on 2.6.17 we get:

fcntl64(3, F_SETSIG, 0x23)              = 0
fcntl64(3, 0x400 /* F_??? */, 0x1)      = 0
nanosleep({50, 0}, 0)                   = ? ERESTART_RESTARTBLOCK (To be 
restarted)
--- SIGIO (I/O possible) @ 0 (0) ---
+++ killed by SIGIO +++

The signal is no longer changed from SIGIO to SIGRT_3.

This causes problems with samba and kernel oplocks.  Windows clients get 
the dreaded "Delayed Write Failed" message when smbd dies with SIGIO.

-- 
Orion Poplawski
System Administrator                   303-415-9701 x222
Colorado Research Associates/NWRA      FAX: 303-415-9702
3380 Mitchell Lane, Boulder CO 80301   http://www.co-ra.com

[-- Attachment #2: oplocktest.c --]
[-- Type: text/x-csrc, Size: 3180 bytes --]

/* 
   Unix SMB/CIFS implementation.
   kernel oplock processing for Linux
   Copyright (C) Andrew Tridgell 2000
   
   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 2 of the License, or
   (at your option) any later version.
   
   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.
   
   You should have received a copy of the GNU General Public License
   along with this program; if not, write to the Free Software
   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/

#define DBGC_CLASS DBGC_LOCKING
#include <signal.h>
#include <errno.h>
#include <sys/fcntl.h>
#include <sys/select.h>
#include <stdio.h>
#include <string.h>
#include <time.h>

typedef unsigned int uint32;

/* these can be removed when they are in glibc headers */
struct  cap_user_header {
	uint32 version;
	int pid;
} header;
struct cap_user_data {
	uint32 effective;
	uint32 permitted;
	uint32 inheritable;
} data;

extern int capget(struct cap_user_header * hdrp,
		  struct cap_user_data * datap);
extern int capset(struct cap_user_header * hdrp,
		  const struct cap_user_data * datap);

#ifndef F_SETLEASE
#define F_SETLEASE	1024
#endif

#ifndef F_GETLEASE
#define F_GETLEASE	1025
#endif

#ifndef CAP_LEASE
#define CAP_LEASE 28
#endif

#ifndef RT_SIGNAL_LEASE
#define RT_SIGNAL_LEASE (SIGRTMIN+1)
#endif

#ifndef F_SETSIG
#define F_SETSIG 10
#endif

/****************************************************************************
 Try to gain a linux capability.
****************************************************************************/

static void set_capability(unsigned capability)
{
#ifndef _LINUX_CAPABILITY_VERSION
#define _LINUX_CAPABILITY_VERSION 0x19980330
#endif
	header.version = _LINUX_CAPABILITY_VERSION;
	header.pid = 0;

	if (capget(&header, &data) == -1) {
		printf("Unable to get kernel capabilities (%s)\n", strerror(errno));
		return;
	}

	data.effective |= (1<<capability);

	if (capset(&header, &data) == -1) {
		printf("Unable to set %d capability (%s)\n", 
			 capability, strerror(errno));
	}
}

/****************************************************************************
 Call SETLEASE. If we get EACCES then we try setting up the right capability and
 try again
****************************************************************************/

static int linux_setlease(int fd, int leasetype)
{
	int ret;

	if (fcntl(fd, F_SETSIG, RT_SIGNAL_LEASE) == -1) {
		printf("Failed to set signal handler for kernel lease\n");
		return -1;
	}

	ret = fcntl(fd, F_SETLEASE, leasetype);
	if (ret == -1 && errno == EACCES) {
		set_capability(CAP_LEASE);
		ret = fcntl(fd, F_SETLEASE, leasetype);
	}

	return ret;
}

int main(int argc, char **argv)
{
	int fd;

	fd = open("oplocktest.c", O_RDONLY);
        linux_setlease(fd, 1);	

   struct timespec ts;

   ts.tv_sec = 50;
   ts.tv_nsec = 0;

   nanosleep(&ts, NULL);

   return(0);
}