[PATCH] devfs v181 available

[PATCH] devfs v181 available

[Richard Gooch]

  Hi, all. Version 181 of my devfs patch is now available from:
http://www.atnf.csiro.au/~rgooch/linux/kernel-patches.html
The devfs FAQ is also available here.

Patch directly available from:
ftp://ftp.??.kernel.org/pub/linux/kernel/people/rgooch/v2.4/devfs-patch-current.gz

AND:
ftp://ftp.atnf.csiro.au/pub/people/rgooch/linux/kernel-patches/v2.4/devfs-patch-current.gz

This is against 2.4.6-pre3. Highlights of this release:

- Answered question posed by Al Viro and removed his comments from <devfs_open>

- Moved setting of registered flag after other fields are changed

- Fixed race between <devfsd_close> and <devfsd_notify_one>

- Global VFS changes added bogus BKL to devfsd_close(): removed

- Widened locking in <devfs_readlink> and <devfs_follow_link>

- Replaced <devfsd_read> stack usage with <devfsd_ioctl> kmalloc

- Simplified locking in <devfsd_ioctl> and fixed memory leak

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] devfs v181 available

[Alexander Viro]

On Mon, 18 Jun 2001, Richard Gooch wrote:

> - Widened locking in <devfs_readlink> and <devfs_follow_link>

No, you hadn't. Both vfs_readlink() and vfs_follow_link() are blocking
functions, so BKL is worthless there.

Re: [PATCH] devfs v181 available

[Richard Gooch]

Alexander Viro writes:
> 
> 
> On Mon, 18 Jun 2001, Richard Gooch wrote:
> 
> > - Widened locking in <devfs_readlink> and <devfs_follow_link>
> 
> No, you hadn't. Both vfs_readlink() and vfs_follow_link() are blocking
> functions, so BKL is worthless there.

Huh? The BKL will protect against other operations which might cause
the devfs entry to be unregistered, where those other operations also
grab the BKL. So, it's an improvement.

Sure, some operations may cause unregistration without grabbing the
BKL, but that's orthogonal (and requires more extensive changes). If
this "widening" is of no use, then what use are the existing grabs of
the BKL in those functions? You're the one who added them in the first
place.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] devfs v181 available

[Alexander Viro]

On Mon, 18 Jun 2001, Richard Gooch wrote:

> Alexander Viro writes:
> > 
> > 
> > On Mon, 18 Jun 2001, Richard Gooch wrote:
> > 
> > > - Widened locking in <devfs_readlink> and <devfs_follow_link>
> > 
> > No, you hadn't. Both vfs_readlink() and vfs_follow_link() are blocking
> > functions, so BKL is worthless there.
> 
> Huh? The BKL will protect against other operations which might cause
> the devfs entry to be unregistered, where those other operations also
> grab the BKL. So, it's an improvement.

BKL is released as soon as you block. You _do_ regain it when you get
the next timeslice, but in the meanwhile anything could happen.

> Sure, some operations may cause unregistration without grabbing the

Irrelevant. BKL provides an exclusion only on non-blocking areas.

> BKL, but that's orthogonal (and requires more extensive changes). If
> this "widening" is of no use, then what use are the existing grabs of
> the BKL in those functions? You're the one who added them in the first
> place.

_Moved_ them there from the callers of these functions. And AFAICS you
do need BKL for get_devfs_entry_...(); otherwise relocation of the
table will be able to screw you inside of that function. Now, it will
merrily screw you anyway in a lot of places, but that's another story.

BTW, free advice: when you are checking some condition treat the result
as something that can expire. And don't rely on it past the moment when
it might expired. E.g. in case of de->registered result expires as soon
as you do unlock_kernel() _or_ do anything that might block.

Re: [PATCH] devfs v181 available

[Richard Gooch]

Alexander Viro writes:
> On Mon, 18 Jun 2001, Richard Gooch wrote:
> > Alexander Viro writes:
> > > On Mon, 18 Jun 2001, Richard Gooch wrote:
> > > > - Widened locking in <devfs_readlink> and <devfs_follow_link>
> > > 
> > > No, you hadn't. Both vfs_readlink() and vfs_follow_link() are blocking
> > > functions, so BKL is worthless there.
> > 
> > Huh? The BKL will protect against other operations which might cause
> > the devfs entry to be unregistered, where those other operations also
> > grab the BKL. So, it's an improvement.
> 
> BKL is released as soon as you block. You _do_ regain it when you get
> the next timeslice, but in the meanwhile anything could happen.
> 
> > Sure, some operations may cause unregistration without grabbing the
> 
> Irrelevant. BKL provides an exclusion only on non-blocking areas.

Yeah, I know all that.

> > BKL, but that's orthogonal (and requires more extensive changes). If
> > this "widening" is of no use, then what use are the existing grabs of
> > the BKL in those functions? You're the one who added them in the first
> > place.
> 
> _Moved_ them there from the callers of these functions. And AFAICS
> you do need BKL for get_devfs_entry_...(); otherwise relocation of
> the table will be able to screw you inside of that function. Now, it
> will merrily screw you anyway in a lot of places, but that's another
> story.

OK, so it was another global change.

> BTW, free advice: when you are checking some condition treat the
> result as something that can expire. And don't rely on it past the
> moment when it might expired. E.g. in case of de->registered result
> expires as soon as you do unlock_kernel() _or_ do anything that
> might block.

Yeah, I know. Fear not: I'll be adding proper locks (spinlocks and
semaphores) to the code, continuing the work I started this weekend. I
don't like the BKL anyway (too coarse grained), and hope to see it
removed entirely one day.

Question: assuming data fed to vfs_follow_link() is "safe", does it
need the BKL? I can see that vfs_readlink() obviously doesn't need
it. From reading Documentation/filesystems/Locking I suspect it
doesn't need the BKL, but the way I read it says "follow_link() method
does not *have* the BKL already". But that doesn't explicitely say
whether vfs_follow_link() needs it.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] devfs v181 available

[Alexander Viro]

On Mon, 18 Jun 2001, Richard Gooch wrote:

> > Irrelevant. BKL provides an exclusion only on non-blocking areas.
> 
> Yeah, I know all that.

So what the hell are you talking about?

> > _Moved_ them there from the callers of these functions. And AFAICS
> > you do need BKL for get_devfs_entry_...(); otherwise relocation of
> > the table will be able to screw you inside of that function. Now, it
> > will merrily screw you anyway in a lot of places, but that's another
> > story.
> 
> OK, so it was another global change.

Moving BKL into the ->readlink() and ->follow_link()? Sure, it was a global
change. About a year ago.

> Question: assuming data fed to vfs_follow_link() is "safe", does it
            ^^^^^^^^
> need the BKL? I can see that vfs_readlink() obviously doesn't need
> it. From reading Documentation/filesystems/Locking I suspect it
> doesn't need the BKL, but the way I read it says "follow_link() method
> does not *have* the BKL already". But that doesn't explicitely say
> whether vfs_follow_link() needs it.

vfs_follow_link() doesn't need it. Moreover, if data fed to it is unsafe
without BKL, you are screwed even if you take BKL. So assumption above
is bogus - you _never_ need BKL on that call.

Re: [PATCH] devfs v181 available

[Richard Gooch]

Alexander Viro writes:
> 
> 
> On Mon, 18 Jun 2001, Richard Gooch wrote:
> 
> > > Irrelevant. BKL provides an exclusion only on non-blocking areas.
> > 
> > Yeah, I know all that.
> 
> So what the hell are you talking about?

Never mind. We seem to be talking at cross purposes. We both know how
the BKL works and the implications, so there's not much point beating
our heads trying to communicate redundant information :-)

> > > _Moved_ them there from the callers of these functions. And AFAICS
> > > you do need BKL for get_devfs_entry_...(); otherwise relocation of
> > > the table will be able to screw you inside of that function. Now, it
> > > will merrily screw you anyway in a lot of places, but that's another
> > > story.
> > 
> > OK, so it was another global change.
> 
> Moving BKL into the ->readlink() and ->follow_link()? Sure, it was a global
> change. About a year ago.
> 
> > Question: assuming data fed to vfs_follow_link() is "safe", does it
>             ^^^^^^^^
> > need the BKL? I can see that vfs_readlink() obviously doesn't need
> > it. From reading Documentation/filesystems/Locking I suspect it
> > doesn't need the BKL, but the way I read it says "follow_link() method
> > does not *have* the BKL already". But that doesn't explicitely say
> > whether vfs_follow_link() needs it.
> 
> vfs_follow_link() doesn't need it. Moreover, if data fed to it is
> unsafe without BKL, you are screwed even if you take BKL. So
> assumption above is bogus - you _never_ need BKL on that call.

OK, you didn't see what I was driving at. If I had said "if my data is
protected by a semaphore, do I still need the BKL for
vfs_follow_link()" I guess it would have been clearer. Anyway, you've
answered my question, thanks.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca