Hi,
the patch below makes flock_lock_file_wait static, because it is only used
(once) in fs/locks.c. Making it static allows gcc to generate better code
(partial or entirely inlining it, gcc 3.4 also optimizes the calling
convention for static functions which are guaranteed only local to the file)
Signed-off-by: Arjan van de Ven <[email protected]>
diff -purN linux-2.6.10/fs/locks.c linux/fs/locks.c
--- linux-2.6.10/fs/locks.c 2005-01-09 14:51:10.101125171 +0100
+++ linux/fs/locks.c 2005-01-09 20:39:44.930216959 +0100
@@ -1451,7 +1451,7 @@ out_unlock:
*
* Add a FLOCK style lock to a file.
*/
-int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
+static int flock_lock_file_wait(struct file *filp, struct file_lock *fl)
{
int error;
might_sleep();
@@ -1469,8 +1469,6 @@ int flock_lock_file_wait(struct file *fi
return error;
}
-EXPORT_SYMBOL(flock_lock_file_wait);
-
/**
* sys_flock: - flock() system call.
* @fd: the file descriptor to lock.
diff -purN linux-2.6.10/include/linux/fs.h linux/include/linux/fs.h
--- linux-2.6.10/include/linux/fs.h 2005-01-09 14:51:10.293101584 +0100
+++ linux/include/linux/fs.h 2005-01-09 20:39:18.678389875 +0100
@@ -709,7 +709,6 @@ extern int posix_lock_file_wait(struct f
extern void posix_block_lock(struct file_lock *, struct file_lock *);
extern void posix_unblock_lock(struct file *, struct file_lock *);
extern int posix_locks_deadlock(struct file_lock *, struct file_lock *);
-extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
extern int __break_lease(struct inode *inode, unsigned int flags);
extern void lease_get_mtime(struct inode *, struct timespec *time);
extern int setlease(struct file *, long, struct file_lock **);
su den 09.01.2005 Klokka 19:42 (+0000) skreiv Arjan van de Ven:
> Hi,
>
> the patch below makes flock_lock_file_wait static, because it is only used
> (once) in fs/locks.c. Making it static allows gcc to generate better code
> (partial or entirely inlining it, gcc 3.4 also optimizes the calling
> convention for static functions which are guaranteed only local to the file)
Veto. That function is also there for those filesystems that need to
mirror their locks in the VFS. I believe the GFS people are already
using it (they implemented all this anyway), and sooner or later, NFS is
going to have to do it too...
Cheers,
Trond
--
Trond Myklebust <[email protected]>
On Sun, 2005-01-09 at 17:44 -0500, Trond Myklebust wrote:
> su den 09.01.2005 Klokka 19:42 (+0000) skreiv Arjan van de Ven:
> > Hi,
> >
> > the patch below makes flock_lock_file_wait static, because it is only used
> > (once) in fs/locks.c. Making it static allows gcc to generate better code
> > (partial or entirely inlining it, gcc 3.4 also optimizes the calling
> > convention for static functions which are guaranteed only local to the file)
>
> Veto. That function is also there for those filesystems that need to
> mirror their locks in the VFS. I believe the GFS people are already
> using it (they implemented all this anyway), and sooner or later, NFS is
> going to have to do it too...
before
# size fs/locks.o
text data bss dec hex filename
14712 48 4 14764 39ac fs/locks.o
after (with static inline)
# size fs/locks.o
text data bss dec hex filename
14648 48 4 14700 396c fs/locks.o
is "sooner or later" and "maybe someone else uses it" worth making
everyone elses kernel bigger by 500 bytes of code ?
On Sun, Jan 09, 2005 at 05:44:10PM -0500, Trond Myklebust wrote:
> su den 09.01.2005 Klokka 19:42 (+0000) skreiv Arjan van de Ven:
> > Hi,
> >
> > the patch below makes flock_lock_file_wait static, because it is only used
> > (once) in fs/locks.c. Making it static allows gcc to generate better code
> > (partial or entirely inlining it, gcc 3.4 also optimizes the calling
> > convention for static functions which are guaranteed only local to the file)
>
> Veto. That function is also there for those filesystems that need to
> mirror their locks in the VFS. I believe the GFS people are already
> using it (they implemented all this anyway), and sooner or later, NFS is
> going to have to do it too...
I second the veto. GFS does use this interface.
--
Ken Preslan <[email protected]>
On Mon, 2005-01-10 at 09:19 +0100, Arjan van de Ven wrote:
> On Sun, 2005-01-09 at 17:44 -0500, Trond Myklebust wrote:
> > su den 09.01.2005 Klokka 19:42 (+0000) skreiv Arjan van de Ven:
> > > Hi,
> > >
> > > the patch below makes flock_lock_file_wait static, because it is only used
> > > (once) in fs/locks.c. Making it static allows gcc to generate better code
> > > (partial or entirely inlining it, gcc 3.4 also optimizes the calling
> > > convention for static functions which are guaranteed only local to the file)
> >
> > Veto. That function is also there for those filesystems that need to
> > mirror their locks in the VFS. I believe the GFS people are already
> > using it (they implemented all this anyway), and sooner or later, NFS is
> > going to have to do it too...
>
> before
> # size fs/locks.o
> text data bss dec hex filename
> 14712 48 4 14764 39ac fs/locks.o
>
> after (with static inline)
> # size fs/locks.o
> text data bss dec hex filename
> 14648 48 4 14700 396c fs/locks.o
>
>
> is "sooner or later" and "maybe someone else uses it" worth making
> everyone elses kernel bigger by 500 bytes of code ?
eh 60 not 500; sorry need coffee
On Mon, 2005-01-10 at 02:35 -0600, Ken Preslan wrote:
> On Sun, Jan 09, 2005 at 05:44:10PM -0500, Trond Myklebust wrote:
> > su den 09.01.2005 Klokka 19:42 (+0000) skreiv Arjan van de Ven:
> > > Hi,
> > >
> > > the patch below makes flock_lock_file_wait static, because it is only used
> > > (once) in fs/locks.c. Making it static allows gcc to generate better code
> > > (partial or entirely inlining it, gcc 3.4 also optimizes the calling
> > > convention for static functions which are guaranteed only local to the file)
> >
> > Veto. That function is also there for those filesystems that need to
> > mirror their locks in the VFS. I believe the GFS people are already
> > using it (they implemented all this anyway), and sooner or later, NFS is
> > going to have to do it too...
>
> I second the veto. GFS does use this interface.
>
when are you going to propose GFS for inclusion? Is that next week or is
that months out? Is that really worth bloating everyones kernel until
then ?
m? den 10.01.2005 Klokka 09:38 (+0100) skreiv Arjan van de Ven:
>
> > is "sooner or later" and "maybe someone else uses it" worth making
> > everyone elses kernel bigger by 500 bytes of code ?
>
> eh 60 not 500; sorry need coffee
It's an API that provides *necessary* functionality for those
filesystems that wish to override the standard flock(). It was very
recently introduced by a third party, so we haven't had time to code up
an NFS flock yet.
Removing it now will just mean that we have to reintroduce it in a month
or so when NFS and the other filesystems start to catch up.
Cheers,
Trond
--
Trond Myklebust <[email protected]>
On Mon, 2005-01-10 at 09:23 -0500, Trond Myklebust wrote:
> må den 10.01.2005 Klokka 09:38 (+0100) skreiv Arjan van de Ven:
> >
> > > is "sooner or later" and "maybe someone else uses it" worth making
> > > everyone elses kernel bigger by 500 bytes of code ?
> >
> > eh 60 not 500; sorry need coffee
>
> It's an API that provides *necessary* functionality for those
> filesystems that wish to override the standard flock(). It was very
> recently introduced by a third party, so we haven't had time to code up
> an NFS flock yet.
where "recently" is last september....
bloating the kernel unused since then...
> Removing it now will just mean that we have to reintroduce it in a month
> or so when NFS and the other filesystems start to catch up.
if NFS indeed is going to use this in a month then you're probably
right, bloating all kernels for another few weeks (before 2.6.11 is out
anyway) isn't a big deal. I'm looking forward to nfs using this in this
timeframe...
If it is going to take a LOT longer though I still feel it's wrong to
bloat *everyones* kernel with this stuff.
(you may think "it's only 100 bytes", well, there are 700+ other such
functions, total that makes over at least 70Kb of unswappable, wasted
memory if not more.)
ty den 11.01.2005 Klokka 09:31 (+0100) skreiv Arjan van de Ven:
> On Mon, 2005-01-10 at 09:23 -0500, Trond Myklebust wrote:
> > m? den 10.01.2005 Klokka 09:38 (+0100) skreiv Arjan van de Ven:
> > >
> > > > is "sooner or later" and "maybe someone else uses it" worth making
> > > > everyone elses kernel bigger by 500 bytes of code ?
> > >
> > > eh 60 not 500; sorry need coffee
> >
> > It's an API that provides *necessary* functionality for those
> > filesystems that wish to override the standard flock(). It was very
> > recently introduced by a third party, so we haven't had time to code up
> > an NFS flock yet.
>
> where "recently" is last september....
> bloating the kernel unused since then...
Feel free to help out if you think the NFS development effort is
understaffed.
> If it is going to take a LOT longer though I still feel it's wrong to
> bloat *everyones* kernel with this stuff.
>
> (you may think "it's only 100 bytes", well, there are 700+ other such
> functions, total that makes over at least 70Kb of unswappable, wasted
> memory if not more.)
A list of these 700+ unused exported APIs would be very useful so that
we can deprecate and/or get rid of them.
Concerning this case, though, and to make what I said in the earlier
mails (a lot) more explicit.
If you unexport flock_lock_file_wait(), then you might as well back out
the entire bloody ->flock() changeset instead because keeping the
->flock() VFS override support without the functionality to make
implementation practical (which is what you appear to want to do) is a
waste of more than 70 bytes of memory.
Now please go and figure out what it is you actually want to do here.
Trond
--
Trond Myklebust <[email protected]>
On Tue, 2005-01-11 at 14:16 -0500, Trond Myklebust wrote:
> > (you may think "it's only 100 bytes", well, there are 700+ other such
> > functions, total that makes over at least 70Kb of unswappable, wasted
> > memory if not more.)
>
> A list of these 700+ unused exported APIs would be very useful so that
> we can deprecate and/or get rid of them.
http://people.redhat.com/arjanv/unused
has the list of symbols that are unused on an i386 allmodconfig based on
the -bk tree 2 days ago.
> Concerning this case, though, and to make what I said in the earlier
> mails (a lot) more explicit.
>
> If you unexport flock_lock_file_wait(), then you might as well back out
> the entire bloody ->flock() changeset instead because keeping the
> ->flock() VFS override support without the functionality to make
> implementation practical (which is what you appear to want to do) is a
> waste of more than 70 bytes of memory.
>
> Now please go and figure out what it is you actually want to do here.
save space most of all, and reduce bloat that is not used.
Again if you're going to use it soon, fine. If not, as you say, the
entire thing should probably go because it's a bunch of unused code,
additions to data structures and conditional branches.
On Tue, Jan 11, 2005 at 02:16:44PM -0500, Trond Myklebust wrote:
> ty den 11.01.2005 Klokka 09:31 (+0100) skreiv Arjan van de Ven:
>...
> > If it is going to take a LOT longer though I still feel it's wrong to
> > bloat *everyones* kernel with this stuff.
> >
> > (you may think "it's only 100 bytes", well, there are 700+ other such
> > functions, total that makes over at least 70Kb of unswappable, wasted
> > memory if not more.)
>
> A list of these 700+ unused exported APIs would be very useful so that
> we can deprecate and/or get rid of them.
>...
Patches are already sent for each one that is found (the one by Arjan in
this discusison is one of them).
My figures in [1] show, any kind of deprecation would mean _much_ extra
work within the current 2.6 development model.
> Trond
cu
Adrian
[1] http://www.ussg.iu.edu/hypermail/linux/kernel/0501.0/2036.html
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
lau den 15.01.2005 Klokka 22:35 (+0100) skreiv Adrian Bunk:
> My figures in [1] show, any kind of deprecation would mean _much_ extra
> work within the current 2.6 development model.
Whereas removal of necessary APIs would not? Thanks...
Cleanups are important, but so is actual development....
--
Trond Myklebust <[email protected]>
On Tue, Jan 11, 2005 at 08:36:22PM +0100, Arjan van de Ven wrote:
> On Tue, 2005-01-11 at 14:16 -0500, Trond Myklebust wrote:
> > > (you may think "it's only 100 bytes", well, there are 700+ other such
> > > functions, total that makes over at least 70Kb of unswappable, wasted
> > > memory if not more.)
> >
> > A list of these 700+ unused exported APIs would be very useful so that
> > we can deprecate and/or get rid of them.
>
> http://people.redhat.com/arjanv/unused
>
> has the list of symbols that are unused on an i386 allmodconfig based on
> the -bk tree 2 days ago.
<donning asbestos suit with the tungsten pinstripes...>
SAN Filesystem is an out-of-tree GPL module that uses the following:
o blk_get_queue(): used to submit I/O requests using the
make_request_fn().
o sock_setsockopt(): used to control communication with other
nodes in the SAN Filesystem.
o vfs_follow_link(): used to interpret symbolic links, which
might point outside of SAN Filesystem.
SDD is a binary module that has committed to get itself to GPL on its
first release after December 31, 2005. It uses:
o __read_lock_failed() and __write_lock_failed(): due to SDD's use
of read_lock() and write_lock(). So, if the plan is to change
read_lock() and write_lock() to do something different, never mind!
So, could the exports for the following symbols from the list please be
retained through December 31, 2005?
blk_get_queue
sock_setsockopt
vfs_follow_link
__read_lock_failed
__write_lock_failed
Thanx, Paul
PS. Yes, there are more than two out-of-tree modules in IBM. Some were
not affected by this list. One is looking carefully at Al Viro's
propagation-node design to see if it does what they need (looks
promising thus far). Still others are having more trouble accepting
the need to stop being binary modules in the near future, but that
is their problem, not yours! ;-) To be fair, at least one in the
last group has some legitimate IP issues, which we are working on.
"Paul E. McKenney" <[email protected]> wrote:
>
> So, could the exports for the following symbols from the list please be
> retained through December 31, 2005?
>
> blk_get_queue
> sock_setsockopt
> vfs_follow_link
> __read_lock_failed
> __write_lock_failed
I don't think there's any plan to unexport any of these, and in most cases
it would be a dopey thing to do anyway. And if we _were_ to try to remove
any of the above exports we should go through the
feature-removal-schedule.txt process.
So I think we're OK?
On Tue, 2005-01-25 at 10:58 -0800, Paul E. McKenney wrote:
> On Tue, Jan 11, 2005 at 08:36:22PM +0100, Arjan van de Ven wrote:
> > On Tue, 2005-01-11 at 14:16 -0500, Trond Myklebust wrote:
> > > > (you may think "it's only 100 bytes", well, there are 700+ other such
> > > > functions, total that makes over at least 70Kb of unswappable, wasted
> > > > memory if not more.)
> > >
> > > A list of these 700+ unused exported APIs would be very useful so that
> > > we can deprecate and/or get rid of them.
> >
> > http://people.redhat.com/arjanv/unused
> >
> > has the list of symbols that are unused on an i386 allmodconfig based on
> > the -bk tree 2 days ago.
>
> <donning asbestos suit with the tungsten pinstripes...>
>
> SAN Filesystem is an out-of-tree GPL module that uses the following:
any plans to submit this for inclusion?
>
> o blk_get_queue(): used to submit I/O requests using the
> make_request_fn().
sounds really like the wrong level, any reason to not use submit_bio /
submit_bh instead? Every piece of code outside the core block layer that
I've seen that tries to do this has been wrong/broken to date.
>
> o sock_setsockopt(): used to control communication with other
> nodes in the SAN Filesystem.
>
again this very much looks like a misuse; sock_setsocketopt() gets a
*userspace* pointer as argument. Bad API to use (and if you look at
CIFS, they would also like a real nice internal api instead, but don't
use sock_setsockopt() since it's the wrong api)
> SDD is a binary module that has committed to get itself to GPL on its
> first release after December 31, 2005. It uses:
>
> o __read_lock_failed() and __write_lock_failed(): due to SDD's use
> of read_lock() and write_lock(). So, if the plan is to change
> read_lock() and write_lock() to do something different, never mind!
those two exports are "internal" following from copying the
implementation of read_lock() into the code before compiling it (by the
preprocessor) and currently of course won't go away unless readlocks
change/go away.
Another question: is the SDD module even available for mainline kernels,
or is it only available for distribution kernels ?
> o vfs_follow_link(): used to interpret symbolic links, which
> might point outside of SAN Filesystem.
This one is going away very soon, including the whole old-style
->follow_link support - for technical reasons.
Please convert your driver to put the contents of the symlink into
... and implement ->put_link like all intree filesystems already did.
Without that we can't bump the limit on recursive symlinks, a feature
that IBM has been pushing for very hard, btw..
On Wed, Jan 26, 2005 at 09:43:08AM +0000, Christoph Hellwig wrote:
> > o vfs_follow_link(): used to interpret symbolic links, which
> > might point outside of SAN Filesystem.
>
> This one is going away very soon, including the whole old-style
> ->follow_link support - for technical reasons.
Not really. In some cases it _is_ legitimate. Trivial example:
/proc/self.
On Wed, Jan 26, 2005 at 09:51:31AM +0000, Al Viro wrote:
> On Wed, Jan 26, 2005 at 09:43:08AM +0000, Christoph Hellwig wrote:
> > > o vfs_follow_link(): used to interpret symbolic links, which
> > > might point outside of SAN Filesystem.
> >
> > This one is going away very soon, including the whole old-style
> > ->follow_link support - for technical reasons.
>
> Not really. In some cases it _is_ legitimate. Trivial example:
> /proc/self.
Which is an extreme special case an not modular.
On Wed, Jan 26, 2005 at 09:55:04AM +0000, Christoph Hellwig wrote:
> On Wed, Jan 26, 2005 at 09:51:31AM +0000, Al Viro wrote:
> > On Wed, Jan 26, 2005 at 09:43:08AM +0000, Christoph Hellwig wrote:
> > > > o vfs_follow_link(): used to interpret symbolic links, which
> > > > might point outside of SAN Filesystem.
> > >
> > > This one is going away very soon, including the whole old-style
> > > ->follow_link support - for technical reasons.
> >
> > Not really. In some cases it _is_ legitimate. Trivial example:
> > /proc/self.
>
> Which is an extreme special case an not modular.
I'm not convinced. Sure, leaving traversal to caller is preferable.
I don't believe that it should be enforced. That's why we have a way
to enforce a separate limit on that sort of recursion...
On Wed, Jan 26, 2005 at 10:01:00AM +0100, Arjan van de Ven wrote:
> On Tue, 2005-01-25 at 10:58 -0800, Paul E. McKenney wrote:
> > On Tue, Jan 11, 2005 at 08:36:22PM +0100, Arjan van de Ven wrote:
> > > On Tue, 2005-01-11 at 14:16 -0500, Trond Myklebust wrote:
> > > > > (you may think "it's only 100 bytes", well, there are 700+ other such
> > > > > functions, total that makes over at least 70Kb of unswappable, wasted
> > > > > memory if not more.)
> > > >
> > > > A list of these 700+ unused exported APIs would be very useful so that
> > > > we can deprecate and/or get rid of them.
> > >
> > > http://people.redhat.com/arjanv/unused
> > >
> > > has the list of symbols that are unused on an i386 allmodconfig based on
> > > the -bk tree 2 days ago.
> >
> > <donning asbestos suit with the tungsten pinstripes...>
> >
> > SAN Filesystem is an out-of-tree GPL module that uses the following:
>
> any plans to submit this for inclusion?
Sigh! Given that the core of the SAN Filesystem client is a C++
module that runs under Linux, AIX, Windows, &c, I don't have great
hopes for its being accepted for inclusion. :-(
> > o blk_get_queue(): used to submit I/O requests using the
> > make_request_fn().
>
> sounds really like the wrong level, any reason to not use submit_bio /
> submit_bh instead? Every piece of code outside the core block layer that
> I've seen that tries to do this has been wrong/broken to date.
I will have them look into this possibility.
> > o sock_setsockopt(): used to control communication with other
> > nodes in the SAN Filesystem.
>
> again this very much looks like a misuse; sock_setsocketopt() gets a
> *userspace* pointer as argument. Bad API to use (and if you look at
> CIFS, they would also like a real nice internal api instead, but don't
> use sock_setsockopt() since it's the wrong api)
A better API would indeed be a good thing! I tried a quick search to
find a discussion, but came up dry. If you have a pointer or contact,
please let me know. I am also checking with the CIFS people that I know.
> > SDD is a binary module that has committed to get itself to GPL on its
> > first release after December 31, 2005. It uses:
> >
> > o __read_lock_failed() and __write_lock_failed(): due to SDD's use
> > of read_lock() and write_lock(). So, if the plan is to change
> > read_lock() and write_lock() to do something different, never mind!
>
> those two exports are "internal" following from copying the
> implementation of read_lock() into the code before compiling it (by the
> preprocessor) and currently of course won't go away unless readlocks
> change/go away.
OK, sounds good!
> Another question: is the SDD module even available for mainline kernels,
> or is it only available for distribution kernels ?
Distributions only.
Thanx, Paul
On Wed, 2005-01-26 at 08:07 -0800, Paul E. McKenney wrote:
> > Another question: is the SDD module even available for mainline kernels,
> > or is it only available for distribution kernels ?
>
> Distributions only.
don't you think it's a bit weird/offensive to ask for exports in the
mainline kernel.org kernel while all you care about is certain vendor
kernels and never plan to provide it for mainline????
On Wed, Jan 26, 2005 at 07:59:42PM +0100, Arjan van de Ven wrote:
> On Wed, 2005-01-26 at 08:07 -0800, Paul E. McKenney wrote:
>
> > > Another question: is the SDD module even available for mainline kernels,
> > > or is it only available for distribution kernels ?
> >
> > Distributions only.
>
> don't you think it's a bit weird/offensive to ask for exports in the
> mainline kernel.org kernel while all you care about is certain vendor
> kernels and never plan to provide it for mainline????
In my experience, the only way to get exports into a major distribution
is to get them into mainline kernel.org. If you can get Red Hat to
change its stance on this, works for me!
Thanx, Paul
On Fri, Jan 28, 2005 at 06:14:46AM -0800, Paul E. McKenney wrote:
> In my experience, the only way to get exports into a major distribution
> is to get them into mainline kernel.org. If you can get Red Hat to
> change its stance on this, works for me!
That's not the point. You're trying to let us work for you for free
without any gain for us.
On Fri, 2005-01-28 at 06:14 -0800, Paul E. McKenney wrote:
> On Wed, Jan 26, 2005 at 07:59:42PM +0100, Arjan van de Ven wrote:
> > On Wed, 2005-01-26 at 08:07 -0800, Paul E. McKenney wrote:
> >
> > > > Another question: is the SDD module even available for mainline kernels,
> > > > or is it only available for distribution kernels ?
> > >
> > > Distributions only.
> >
> > don't you think it's a bit weird/offensive to ask for exports in the
> > mainline kernel.org kernel while all you care about is certain vendor
> > kernels and never plan to provide it for mainline????
>
> In my experience, the only way to get exports into a major distribution
> is to get them into mainline kernel.org. If you can get Red Hat to
> change its stance on this, works for me!
so a business problem between IBM and Red Hat is reason to bloat
everyones kernel ????
that's sooo wrong.
(and no I cannot change the RH policy)