Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Randy Dunlap]

akpm@linux-foundation.org wrote:
> The mm-of-the-moment snapshot 2009-01-14-20-31 has been uploaded to
> 
>    http://userweb.kernel.org/~akpm/mmotm/
> 
> and will soon be available at
> 
>    git://git.zen-sources.org/zen/mmotm.git


when CONFIG_FILE_LOCKING=n:

mmotm-2009-0114-2031/fs/gfs2/ops_file.c:746: error: 'generic_setlease' undeclared here (not in a function)


since generic_setlease() is a macro/define in that case.

-- 
~Randy

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Steven Whitehouse]

Hi,

On Thu, 2009-01-15 at 11:13 -0800, Randy Dunlap wrote:
> akpm@linux-foundation.org wrote:
> > The mm-of-the-moment snapshot 2009-01-14-20-31 has been uploaded to
> > 
> >    http://userweb.kernel.org/~akpm/mmotm/
> > 
> > and will soon be available at
> > 
> >    git://git.zen-sources.org/zen/mmotm.git
> 
> 
> when CONFIG_FILE_LOCKING=n:
> 
> mmotm-2009-0114-2031/fs/gfs2/ops_file.c:746: error: 'generic_setlease' undeclared here (not in a function)
> 
> 
> since generic_setlease() is a macro/define in that case.
> 
Hmm, it looks like I'll have to do this, in that case:

diff --git a/fs/gfs2/ops_file.c b/fs/gfs2/ops_file.c
index 99d726f..4580335 100644
--- a/fs/gfs2/ops_file.c
+++ b/fs/gfs2/ops_file.c
@@ -743,7 +743,9 @@ const struct file_operations *gfs2_file_fops_nolock = &(const struct file_operat
 	.fsync		= gfs2_fsync,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= generic_file_splice_write,
+#ifdef CONFIG_FILE_LOCKING
 	.setlease	= generic_setlease,
+#endif /* CONFIG_FILE_LOCKING */
 };
 
 const struct file_operations *gfs2_dir_fops_nolock = &(const struct file_operations){


which is not ideal, but I don't see any easy way to avoid the #ifdef,

Steve.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Andrew Morton]

On Fri, 16 Jan 2009 10:20:03 +0000 Steven Whitehouse <swhiteho@redhat.com> wrote:

> Hi,
> 
> On Thu, 2009-01-15 at 11:13 -0800, Randy Dunlap wrote:
> > akpm@linux-foundation.org wrote:
> > > The mm-of-the-moment snapshot 2009-01-14-20-31 has been uploaded to
> > > 
> > >    http://userweb.kernel.org/~akpm/mmotm/
> > > 
> > > and will soon be available at
> > > 
> > >    git://git.zen-sources.org/zen/mmotm.git
> > 
> > 
> > when CONFIG_FILE_LOCKING=n:
> > 
> > mmotm-2009-0114-2031/fs/gfs2/ops_file.c:746: error: 'generic_setlease' undeclared here (not in a function)
> > 
> > 
> > since generic_setlease() is a macro/define in that case.
> > 
> Hmm, it looks like I'll have to do this, in that case:
> 
> diff --git a/fs/gfs2/ops_file.c b/fs/gfs2/ops_file.c
> index 99d726f..4580335 100644
> --- a/fs/gfs2/ops_file.c
> +++ b/fs/gfs2/ops_file.c
> @@ -743,7 +743,9 @@ const struct file_operations *gfs2_file_fops_nolock = &(const struct file_operat
>  	.fsync		= gfs2_fsync,
>  	.splice_read	= generic_file_splice_read,
>  	.splice_write	= generic_file_splice_write,
> +#ifdef CONFIG_FILE_LOCKING
>  	.setlease	= generic_setlease,
> +#endif /* CONFIG_FILE_LOCKING */
>  };
>  
>  const struct file_operations *gfs2_dir_fops_nolock = &(const struct file_operations){
> 
> 
> which is not ideal, but I don't see any easy way to avoid the #ifdef,
> 

Take a look in fs.h:

#define generic_setlease(a, b, c) ({ -EINVAL; })

If that wasn't a stupid macro, your code would have compiled and ran
just as intended.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Steven Whitehouse]

Hi,

On Fri, 2009-01-16 at 08:43 -0800, Andrew Morton wrote:
> On Fri, 16 Jan 2009 10:20:03 +0000 Steven Whitehouse <swhiteho@redhat.com> wrote:
> 
> > Hi,
> > 
> > On Thu, 2009-01-15 at 11:13 -0800, Randy Dunlap wrote:
> > > akpm@linux-foundation.org wrote:
> > > > The mm-of-the-moment snapshot 2009-01-14-20-31 has been uploaded to
> > > > 
> > > >    http://userweb.kernel.org/~akpm/mmotm/
> > > > 
> > > > and will soon be available at
> > > > 
> > > >    git://git.zen-sources.org/zen/mmotm.git
> > > 
> > > 
> > > when CONFIG_FILE_LOCKING=n:
> > > 
> > > mmotm-2009-0114-2031/fs/gfs2/ops_file.c:746: error: 'generic_setlease' undeclared here (not in a function)
> > > 
> > > 
> > > since generic_setlease() is a macro/define in that case.
> > > 
> > Hmm, it looks like I'll have to do this, in that case:
> > 
> > diff --git a/fs/gfs2/ops_file.c b/fs/gfs2/ops_file.c
> > index 99d726f..4580335 100644
> > --- a/fs/gfs2/ops_file.c
> > +++ b/fs/gfs2/ops_file.c
> > @@ -743,7 +743,9 @@ const struct file_operations *gfs2_file_fops_nolock = &(const struct file_operat
> >  	.fsync		= gfs2_fsync,
> >  	.splice_read	= generic_file_splice_read,
> >  	.splice_write	= generic_file_splice_write,
> > +#ifdef CONFIG_FILE_LOCKING
> >  	.setlease	= generic_setlease,
> > +#endif /* CONFIG_FILE_LOCKING */
> >  };
> >  
> >  const struct file_operations *gfs2_dir_fops_nolock = &(const struct file_operations){
> > 
> > 
> > which is not ideal, but I don't see any easy way to avoid the #ifdef,
> > 
> 
> Take a look in fs.h:
> 
> #define generic_setlease(a, b, c) ({ -EINVAL; })
> 
> If that wasn't a stupid macro, your code would have compiled and ran
> just as intended.
> 
There doesn't seem to be an easy answer though. If I #define it to NULL,
that upsets other parts of the code that rely on that macro, and if I
turn it into a inline function which returns -EINVAL, then presumably I
can't take its address for my file_operations.

I could create a small function which then calls generic_setlease I
suppose, but is that any better than this? Its not really very neat
which ever I choose :(

Steve.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Randy Dunlap]

Steven Whitehouse wrote:
> Hi,
> 
> On Fri, 2009-01-16 at 08:43 -0800, Andrew Morton wrote:
>> On Fri, 16 Jan 2009 10:20:03 +0000 Steven Whitehouse <swhiteho@redhat.com> wrote:
>>
>>> Hi,
>>>
>>> On Thu, 2009-01-15 at 11:13 -0800, Randy Dunlap wrote:
>>>> akpm@linux-foundation.org wrote:
>>>>> The mm-of-the-moment snapshot 2009-01-14-20-31 has been uploaded to
>>>>>
>>>>>    http://userweb.kernel.org/~akpm/mmotm/
>>>>>
>>>>> and will soon be available at
>>>>>
>>>>>    git://git.zen-sources.org/zen/mmotm.git
>>>>
>>>> when CONFIG_FILE_LOCKING=n:
>>>>
>>>> mmotm-2009-0114-2031/fs/gfs2/ops_file.c:746: error: 'generic_setlease' undeclared here (not in a function)
>>>>
>>>>
>>>> since generic_setlease() is a macro/define in that case.
>>>>
>>> Hmm, it looks like I'll have to do this, in that case:
>>>
>>> diff --git a/fs/gfs2/ops_file.c b/fs/gfs2/ops_file.c
>>> index 99d726f..4580335 100644
>>> --- a/fs/gfs2/ops_file.c
>>> +++ b/fs/gfs2/ops_file.c
>>> @@ -743,7 +743,9 @@ const struct file_operations *gfs2_file_fops_nolock = &(const struct file_operat
>>>  	.fsync		= gfs2_fsync,
>>>  	.splice_read	= generic_file_splice_read,
>>>  	.splice_write	= generic_file_splice_write,
>>> +#ifdef CONFIG_FILE_LOCKING
>>>  	.setlease	= generic_setlease,
>>> +#endif /* CONFIG_FILE_LOCKING */
>>>  };
>>>  
>>>  const struct file_operations *gfs2_dir_fops_nolock = &(const struct file_operations){
>>>
>>>
>>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
>>>
>> Take a look in fs.h:
>>
>> #define generic_setlease(a, b, c) ({ -EINVAL; })
>>
>> If that wasn't a stupid macro, your code would have compiled and ran
>> just as intended.
>>
> There doesn't seem to be an easy answer though. If I #define it to NULL,
> that upsets other parts of the code that rely on that macro, and if I
> turn it into a inline function which returns -EINVAL, then presumably I
> can't take its address for my file_operations.

No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
I've seen a few cases of that).

> I could create a small function which then calls generic_setlease I
> suppose, but is that any better than this? Its not really very neat
> which ever I choose :(


-- 
~Randy

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Andrew Morton]

On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:

> >>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
> >>>
> >> Take a look in fs.h:
> >>
> >> #define generic_setlease(a, b, c) ({ -EINVAL; })
> >>
> >> If that wasn't a stupid macro, your code would have compiled and ran
> >> just as intended.
> >>
> > There doesn't seem to be an easy answer though. If I #define it to NULL,
> > that upsets other parts of the code that rely on that macro, and if I
> > turn it into a inline function which returns -EINVAL, then presumably I
> > can't take its address for my file_operations.
> 
> No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
> I've seen a few cases of that).
> 

yup.  It measn that we'll get a separate private copy of the
generic_setlease() code in each compilation unit which takes its
address, but I don't think that would kill us.

The prevention is of course to put the stub function in a core kernel
.c file and export it to modules.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Steven Whitehouse]

Hi,

On Fri, 2009-01-16 at 09:35 -0800, Andrew Morton wrote:
> On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> > >>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
> > >>>
> > >> Take a look in fs.h:
> > >>
> > >> #define generic_setlease(a, b, c) ({ -EINVAL; })
> > >>
> > >> If that wasn't a stupid macro, your code would have compiled and ran
> > >> just as intended.
> > >>
> > > There doesn't seem to be an easy answer though. If I #define it to NULL,
> > > that upsets other parts of the code that rely on that macro, and if I
> > > turn it into a inline function which returns -EINVAL, then presumably I
> > > can't take its address for my file_operations.
> > 
> > No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
> > I've seen a few cases of that).
> > 
> 
> yup.  It measn that we'll get a separate private copy of the
> generic_setlease() code in each compilation unit which takes its
> address, but I don't think that would kill us.
> 
> The prevention is of course to put the stub function in a core kernel
> .c file and export it to modules.
> 
Ok, I'll have another look at this and try and cook something up,

Steve.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Steven Whitehouse]

Hi,

On Fri, 2009-01-16 at 09:35 -0800, Andrew Morton wrote:
> On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> > >>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
> > >>>
> > >> Take a look in fs.h:
> > >>
> > >> #define generic_setlease(a, b, c) ({ -EINVAL; })
> > >>
> > >> If that wasn't a stupid macro, your code would have compiled and ran
> > >> just as intended.
> > >>
> > > There doesn't seem to be an easy answer though. If I #define it to NULL,
> > > that upsets other parts of the code that rely on that macro, and if I
> > > turn it into a inline function which returns -EINVAL, then presumably I
> > > can't take its address for my file_operations.
> > 
> > No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
> > I've seen a few cases of that).
> > 
> 
> yup.  It measn that we'll get a separate private copy of the
> generic_setlease() code in each compilation unit which takes its
> address, but I don't think that would kill us.
> 
> The prevention is of course to put the stub function in a core kernel
> .c file and export it to modules.
> 
Having looked into this in a bit more detail now, it seems that this
particular function (generic_setlease) is one of a number appearing in
fs.h which are replaced by macros in the case that CONFIG_FILE_LOCKING
is not set.

So rather than just do the one function, it seemed to make sense to me
to make them all the same. So this uses inline functions as originally
proposed. If you'd prefer that we don't inline them and instead have a
fs/no-locks.c or something like that with stub functions in it, then I"m
happy to revise the patch accordingly.

This patch passes my compile tests, modulo a small change that I need to
make in GFS2 to suppress a warning (not attached). That seems to be
related to yet another set of macros which appear only with
CONFIG_FILE_LOCKING not set. Maybe I should update those to be inline
functions as well....

Steve.

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6022f44..392c18a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1057,34 +1057,147 @@ extern int lease_modify(struct file_lock **, int);
 extern int lock_may_read(struct inode *, loff_t start, unsigned long count);
 extern int lock_may_write(struct inode *, loff_t start, unsigned long count);
 #else /* !CONFIG_FILE_LOCKING */
-#define fcntl_getlk(a, b) ({ -EINVAL; })
-#define fcntl_setlk(a, b, c, d) ({ -EACCES; })
+static inline int fcntl_getlk(struct file *file, struct flock __user *user)
+{
+	return -EINVAL;
+}
+
+static inline int fcntl_setlk(unsigned int fd, struct file *file,
+			      unsigned int cmd, struct flock __user *user)
+{
+	return -EACCES;
+}
+
 #if BITS_PER_LONG == 32
-#define fcntl_getlk64(a, b) ({ -EINVAL; })
-#define fcntl_setlk64(a, b, c, d) ({ -EACCES; })
+static inline int fcntl_getlk64(struct file *file, struct flock64 __user *user)
+{
+	return -EINVAL;
+}
+
+static inline int fcntl_setlk64(unsigned int fd, struct file *file,
+				unsigned int cmd, struct flock64 __user *user)
+{
+	return -EACCES;
+}
 #endif
-#define fcntl_setlease(a, b, c) ({ 0; })
-#define fcntl_getlease(a) ({ 0; })
-#define locks_init_lock(a) ({ })
-#define __locks_copy_lock(a, b) ({ })
-#define locks_copy_lock(a, b) ({ })
-#define locks_remove_posix(a, b) ({ })
-#define locks_remove_flock(a) ({ })
-#define posix_test_lock(a, b) ({ 0; })
-#define posix_lock_file(a, b, c) ({ -ENOLCK; })
-#define posix_lock_file_wait(a, b) ({ -ENOLCK; })
-#define posix_unblock_lock(a, b) (-ENOENT)
-#define vfs_test_lock(a, b) ({ 0; })
-#define vfs_lock_file(a, b, c, d) (-ENOLCK)
-#define vfs_cancel_lock(a, b) ({ 0; })
-#define flock_lock_file_wait(a, b) ({ -ENOLCK; })
-#define __break_lease(a, b) ({ 0; })
-#define lease_get_mtime(a, b) ({ })
-#define generic_setlease(a, b, c) ({ -EINVAL; })
-#define vfs_setlease(a, b, c) ({ -EINVAL; })
-#define lease_modify(a, b) ({ -EINVAL; })
-#define lock_may_read(a, b, c) ({ 1; })
-#define lock_may_write(a, b, c) ({ 1; })
+static inline int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
+{
+	return 0;
+}
+
+static inline int fcntl_getlease(struct file *filp)
+{
+	return 0;
+}
+
+static inline void locks_init_lock(struct file_lock *fl)
+{
+	return;
+}
+
+static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
+{
+	return;
+}
+
+static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
+{
+	return;
+}
+
+static inline void locks_remove_posix(struct file *filp, fl_owner_t owner)
+{
+	return;
+}
+
+static inline void locks_remove_flock(struct file *filp)
+{
+	return;
+}
+
+static inline void posix_test_lock(struct file *filp, struct file_lock *fl)
+{
+	return;
+}
+
+static inline int posix_lock_file(struct file *filp, struct file_lock *fl,
+				  struct file_lock *conflock)
+{
+	return -ENOLCK;
+}
+
+static inline int posix_lock_file_wait(struct file *filp, struct file_lock *fl)
+{
+	return -ENOLCK;
+}
+
+static inline int posix_unblock_lock(struct file *filp,
+				     struct file_lock *waiter)
+{
+	return -ENOENT;
+}
+
+static inline int vfs_test_lock(struct file *filp, struct file_lock *fl)
+{
+	return 0;
+}
+
+static inline int vfs_lock_file(struct file *filp, unsigned int cmd,
+				struct file_lock *fl, struct file_lock *conf)
+{
+	return -ENOLCK;
+}
+
+static inline int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
+{
+	return 0;
+}
+
+static inline int flock_lock_file_wait(struct file *filp,
+				       struct file_lock *request)
+{
+	return -ENOLCK;
+}
+
+static inline int __break_lease(struct inode *inode, unsigned int mode)
+{
+	return 0;
+}
+
+static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
+{
+	return;
+}
+
+static inline int generic_setlease(struct file *filp, long arg,
+				    struct file_lock **flp)
+{
+	return -EINVAL;
+}
+
+static inline int vfs_setlease(struct file *filp, long arg,
+			       struct file_lock **lease)
+{
+	return -EINVAL;
+}
+
+static inline int lease_modify(struct file_lock **before, int arg)
+{
+	return -EINVAL;
+}
+
+static inline int lock_may_read(struct inode *inode, loff_t start,
+				unsigned long len)
+{
+	return 1;
+}
+
+static inline int lock_may_write(struct inode *inode, loff_t start,
+				 unsigned long len)
+{
+	return 1;
+}
+
 #endif /* !CONFIG_FILE_LOCKING */
 
 

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Randy Dunlap]

Steven Whitehouse wrote:
> Hi,
> 
> On Fri, 2009-01-16 at 09:35 -0800, Andrew Morton wrote:
>> On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
>>
>>>>>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
>>>>>>
>>>>> Take a look in fs.h:
>>>>>
>>>>> #define generic_setlease(a, b, c) ({ -EINVAL; })
>>>>>
>>>>> If that wasn't a stupid macro, your code would have compiled and ran
>>>>> just as intended.
>>>>>
>>>> There doesn't seem to be an easy answer though. If I #define it to NULL,
>>>> that upsets other parts of the code that rely on that macro, and if I
>>>> turn it into a inline function which returns -EINVAL, then presumably I
>>>> can't take its address for my file_operations.
>>> No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
>>> I've seen a few cases of that).
>>>
>> yup.  It measn that we'll get a separate private copy of the
>> generic_setlease() code in each compilation unit which takes its
>> address, but I don't think that would kill us.
>>
>> The prevention is of course to put the stub function in a core kernel
>> .c file and export it to modules.
>>
> Having looked into this in a bit more detail now, it seems that this
> particular function (generic_setlease) is one of a number appearing in
> fs.h which are replaced by macros in the case that CONFIG_FILE_LOCKING
> is not set.
> 
> So rather than just do the one function, it seemed to make sense to me
> to make them all the same. So this uses inline functions as originally
> proposed. If you'd prefer that we don't inline them and instead have a
> fs/no-locks.c or something like that with stub functions in it, then I"m
> happy to revise the patch accordingly.

Acked-by/Tested-by: Randy Dunlap <randy.dunlap@oracle.com>

> This patch passes my compile tests, modulo a small change that I need to
> make in GFS2 to suppress a warning (not attached). That seems to be
> related to yet another set of macros which appear only with
> CONFIG_FILE_LOCKING not set. Maybe I should update those to be inline
> functions as well....

You mean these?  Probably should update them as well.

#else /* !CONFIG_FILE_LOCKING */
#define locks_mandatory_locked(a) ({ 0; })
#define locks_mandatory_area(a, b, c, d, e) ({ 0; })
#define __mandatory_lock(a) ({ 0; })
#define mandatory_lock(a) ({ 0; })
#define locks_verify_locked(a) ({ 0; })
#define locks_verify_truncate(a, b, c) ({ 0; })
#define break_lease(a, b) ({ 0; })
#endif /* CONFIG_FILE_LOCKING */



Thanks.

> Steve.
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6022f44..392c18a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1057,34 +1057,147 @@ extern int lease_modify(struct file_lock **, int);
>  extern int lock_may_read(struct inode *, loff_t start, unsigned long count);
>  extern int lock_may_write(struct inode *, loff_t start, unsigned long count);
>  #else /* !CONFIG_FILE_LOCKING */
> -#define fcntl_getlk(a, b) ({ -EINVAL; })
> -#define fcntl_setlk(a, b, c, d) ({ -EACCES; })
> +static inline int fcntl_getlk(struct file *file, struct flock __user *user)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int fcntl_setlk(unsigned int fd, struct file *file,
> +			      unsigned int cmd, struct flock __user *user)
> +{
> +	return -EACCES;
> +}
> +
>  #if BITS_PER_LONG == 32
> -#define fcntl_getlk64(a, b) ({ -EINVAL; })
> -#define fcntl_setlk64(a, b, c, d) ({ -EACCES; })
> +static inline int fcntl_getlk64(struct file *file, struct flock64 __user *user)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int fcntl_setlk64(unsigned int fd, struct file *file,
> +				unsigned int cmd, struct flock64 __user *user)
> +{
> +	return -EACCES;
> +}
>  #endif
> -#define fcntl_setlease(a, b, c) ({ 0; })
> -#define fcntl_getlease(a) ({ 0; })
> -#define locks_init_lock(a) ({ })
> -#define __locks_copy_lock(a, b) ({ })
> -#define locks_copy_lock(a, b) ({ })
> -#define locks_remove_posix(a, b) ({ })
> -#define locks_remove_flock(a) ({ })
> -#define posix_test_lock(a, b) ({ 0; })
> -#define posix_lock_file(a, b, c) ({ -ENOLCK; })
> -#define posix_lock_file_wait(a, b) ({ -ENOLCK; })
> -#define posix_unblock_lock(a, b) (-ENOENT)
> -#define vfs_test_lock(a, b) ({ 0; })
> -#define vfs_lock_file(a, b, c, d) (-ENOLCK)
> -#define vfs_cancel_lock(a, b) ({ 0; })
> -#define flock_lock_file_wait(a, b) ({ -ENOLCK; })
> -#define __break_lease(a, b) ({ 0; })
> -#define lease_get_mtime(a, b) ({ })
> -#define generic_setlease(a, b, c) ({ -EINVAL; })
> -#define vfs_setlease(a, b, c) ({ -EINVAL; })
> -#define lease_modify(a, b) ({ -EINVAL; })
> -#define lock_may_read(a, b, c) ({ 1; })
> -#define lock_may_write(a, b, c) ({ 1; })
> +static inline int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
> +{
> +	return 0;
> +}
> +
> +static inline int fcntl_getlease(struct file *filp)
> +{
> +	return 0;
> +}
> +
> +static inline void locks_init_lock(struct file_lock *fl)
> +{
> +	return;
> +}
> +
> +static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> +{
> +	return;
> +}
> +
> +static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
> +{
> +	return;
> +}
> +
> +static inline void locks_remove_posix(struct file *filp, fl_owner_t owner)
> +{
> +	return;
> +}
> +
> +static inline void locks_remove_flock(struct file *filp)
> +{
> +	return;
> +}
> +
> +static inline void posix_test_lock(struct file *filp, struct file_lock *fl)
> +{
> +	return;
> +}
> +
> +static inline int posix_lock_file(struct file *filp, struct file_lock *fl,
> +				  struct file_lock *conflock)
> +{
> +	return -ENOLCK;
> +}
> +
> +static inline int posix_lock_file_wait(struct file *filp, struct file_lock *fl)
> +{
> +	return -ENOLCK;
> +}
> +
> +static inline int posix_unblock_lock(struct file *filp,
> +				     struct file_lock *waiter)
> +{
> +	return -ENOENT;
> +}
> +
> +static inline int vfs_test_lock(struct file *filp, struct file_lock *fl)
> +{
> +	return 0;
> +}
> +
> +static inline int vfs_lock_file(struct file *filp, unsigned int cmd,
> +				struct file_lock *fl, struct file_lock *conf)
> +{
> +	return -ENOLCK;
> +}
> +
> +static inline int vfs_cancel_lock(struct file *filp, struct file_lock *fl)
> +{
> +	return 0;
> +}
> +
> +static inline int flock_lock_file_wait(struct file *filp,
> +				       struct file_lock *request)
> +{
> +	return -ENOLCK;
> +}
> +
> +static inline int __break_lease(struct inode *inode, unsigned int mode)
> +{
> +	return 0;
> +}
> +
> +static inline void lease_get_mtime(struct inode *inode, struct timespec *time)
> +{
> +	return;
> +}
> +
> +static inline int generic_setlease(struct file *filp, long arg,
> +				    struct file_lock **flp)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int vfs_setlease(struct file *filp, long arg,
> +			       struct file_lock **lease)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int lease_modify(struct file_lock **before, int arg)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int lock_may_read(struct inode *inode, loff_t start,
> +				unsigned long len)
> +{
> +	return 1;
> +}
> +
> +static inline int lock_may_write(struct inode *inode, loff_t start,
> +				 unsigned long len)
> +{
> +	return 1;
> +}
> +
>  #endif /* !CONFIG_FILE_LOCKING */
>  
>  
> 
> 


-- 
~Randy

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Steven Whitehouse]

Hi,

On Mon, 2009-01-19 at 09:05 -0800, Randy Dunlap wrote:
> Steven Whitehouse wrote:
> > Hi,
> > 
> > On Fri, 2009-01-16 at 09:35 -0800, Andrew Morton wrote:
> >> On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
> >>
> >>>>>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
> >>>>>>
> >>>>> Take a look in fs.h:
> >>>>>
> >>>>> #define generic_setlease(a, b, c) ({ -EINVAL; })
> >>>>>
> >>>>> If that wasn't a stupid macro, your code would have compiled and ran
> >>>>> just as intended.
> >>>>>
> >>>> There doesn't seem to be an easy answer though. If I #define it to NULL,
> >>>> that upsets other parts of the code that rely on that macro, and if I
> >>>> turn it into a inline function which returns -EINVAL, then presumably I
> >>>> can't take its address for my file_operations.
> >>> No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
> >>> I've seen a few cases of that).
> >>>
> >> yup.  It measn that we'll get a separate private copy of the
> >> generic_setlease() code in each compilation unit which takes its
> >> address, but I don't think that would kill us.
> >>
> >> The prevention is of course to put the stub function in a core kernel
> >> .c file and export it to modules.
> >>
> > Having looked into this in a bit more detail now, it seems that this
> > particular function (generic_setlease) is one of a number appearing in
> > fs.h which are replaced by macros in the case that CONFIG_FILE_LOCKING
> > is not set.
> > 
> > So rather than just do the one function, it seemed to make sense to me
> > to make them all the same. So this uses inline functions as originally
> > proposed. If you'd prefer that we don't inline them and instead have a
> > fs/no-locks.c or something like that with stub functions in it, then I"m
> > happy to revise the patch accordingly.
> 
> Acked-by/Tested-by: Randy Dunlap <randy.dunlap@oracle.com>
> 
Ok, Thanks. I'll send a proper patch to a suitable tree. Presumably the
VFS tree would be best for this?

> > This patch passes my compile tests, modulo a small change that I need to
> > make in GFS2 to suppress a warning (not attached). That seems to be
> > related to yet another set of macros which appear only with
> > CONFIG_FILE_LOCKING not set. Maybe I should update those to be inline
> > functions as well....
> 
> You mean these?  Probably should update them as well.
> 
> #else /* !CONFIG_FILE_LOCKING */
> #define locks_mandatory_locked(a) ({ 0; })
> #define locks_mandatory_area(a, b, c, d, e) ({ 0; })
> #define __mandatory_lock(a) ({ 0; })
> #define mandatory_lock(a) ({ 0; })
> #define locks_verify_locked(a) ({ 0; })
> #define locks_verify_truncate(a, b, c) ({ 0; })
> #define break_lease(a, b) ({ 0; })
> #endif /* CONFIG_FILE_LOCKING */
> 
> 
Yes, I'll do a patch for those as well then,

Steve.

Re: mmotm 2009-01-14-20-31 uploaded (gfs2)

[Randy Dunlap]

Steven Whitehouse wrote:
> Hi,
> 
> On Mon, 2009-01-19 at 09:05 -0800, Randy Dunlap wrote:
>> Steven Whitehouse wrote:
>>> Hi,
>>>
>>> On Fri, 2009-01-16 at 09:35 -0800, Andrew Morton wrote:
>>>> On Fri, 16 Jan 2009 09:06:23 -0800 Randy Dunlap <randy.dunlap@oracle.com> wrote:
>>>>
>>>>>>>> which is not ideal, but I don't see any easy way to avoid the #ifdef,
>>>>>>>>
>>>>>>> Take a look in fs.h:
>>>>>>>
>>>>>>> #define generic_setlease(a, b, c) ({ -EINVAL; })
>>>>>>>
>>>>>>> If that wasn't a stupid macro, your code would have compiled and ran
>>>>>>> just as intended.
>>>>>>>
>>>>>> There doesn't seem to be an easy answer though. If I #define it to NULL,
>>>>>> that upsets other parts of the code that rely on that macro, and if I
>>>>>> turn it into a inline function which returns -EINVAL, then presumably I
>>>>>> can't take its address for my file_operations.
>>>>> No, gcc will allow &inline_func and out-of-line it if it is needed (AFAIK;
>>>>> I've seen a few cases of that).
>>>>>
>>>> yup.  It measn that we'll get a separate private copy of the
>>>> generic_setlease() code in each compilation unit which takes its
>>>> address, but I don't think that would kill us.
>>>>
>>>> The prevention is of course to put the stub function in a core kernel
>>>> .c file and export it to modules.
>>>>
>>> Having looked into this in a bit more detail now, it seems that this
>>> particular function (generic_setlease) is one of a number appearing in
>>> fs.h which are replaced by macros in the case that CONFIG_FILE_LOCKING
>>> is not set.
>>>
>>> So rather than just do the one function, it seemed to make sense to me
>>> to make them all the same. So this uses inline functions as originally
>>> proposed. If you'd prefer that we don't inline them and instead have a
>>> fs/no-locks.c or something like that with stub functions in it, then I"m
>>> happy to revise the patch accordingly.
>> Acked-by/Tested-by: Randy Dunlap <randy.dunlap@oracle.com>
>>
> Ok, Thanks. I'll send a proper patch to a suitable tree. Presumably the
> VFS tree would be best for this?

That sounds correct, but no guarantees.

>>> This patch passes my compile tests, modulo a small change that I need to
>>> make in GFS2 to suppress a warning (not attached). That seems to be
>>> related to yet another set of macros which appear only with
>>> CONFIG_FILE_LOCKING not set. Maybe I should update those to be inline
>>> functions as well....
>> You mean these?  Probably should update them as well.
>>
>> #else /* !CONFIG_FILE_LOCKING */
>> #define locks_mandatory_locked(a) ({ 0; })
>> #define locks_mandatory_area(a, b, c, d, e) ({ 0; })
>> #define __mandatory_lock(a) ({ 0; })
>> #define mandatory_lock(a) ({ 0; })
>> #define locks_verify_locked(a) ({ 0; })
>> #define locks_verify_truncate(a, b, c) ({ 0; })
>> #define break_lease(a, b) ({ 0; })
>> #endif /* CONFIG_FILE_LOCKING */
>>
>>
> Yes, I'll do a patch for those as well then,

Thanks.

-- 
~Randy