question about nfs_execute_read: why do we need to do lock_kernel?

question about nfs_execute_read: why do we need to do lock_kernel?

[Xin Zhao]

This question may be dumb. But I am curious why in nfs_execute_read()
function, rpc_execute  is bracketed with lock_kernel() and
unlock_kernel()?

Thanks in advance for your help!

xin

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Trond Myklebust]

On Tue, 2006-04-25 at 00:57 -0400, Xin Zhao wrote:
> This question may be dumb. But I am curious why in nfs_execute_read()
> function, rpc_execute  is bracketed with lock_kernel() and
> unlock_kernel()?

We're keeping the BKL for the moment simply because we are not done
auditing all the RPC code for potential races. When that is done, we
will be able to remove it.

Cheers,
  Trond

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Xin Zhao]

Thanks for your reply. So the only reason is for rpc auditing? If so,
why not just lock the code that updating the audit information? Now
the code is:

lock_kernel()
rpc_execute()
unlock_kernel().

That means the kernel will be blocked when rpc is executed, which
could take long time. Even if rpc_execute() won't take very long, this
implementation still looks inefficient. That's why I am a little
confused on this point.

Any further thought?

Xin


On 4/25/06, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> On Tue, 2006-04-25 at 00:57 -0400, Xin Zhao wrote:
> > This question may be dumb. But I am curious why in nfs_execute_read()
> > function, rpc_execute  is bracketed with lock_kernel() and
> > unlock_kernel()?
>
> We're keeping the BKL for the moment simply because we are not done
> auditing all the RPC code for potential races. When that is done, we
> will be able to remove it.
>
> Cheers,
>   Trond
>
>

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Trond Myklebust]

On Tue, 2006-04-25 at 10:08 -0400, Xin Zhao wrote:
> Thanks for your reply. So the only reason is for rpc auditing? If so,
> why not just lock the code that updating the audit information? Now
> the code is:

No. You misunderstood me.

I said that we need to audit the _code_ for races.

In the distant past, the rpc code was entirely protected by the BKL
against concurrent access to shared objects (queues, structures,
variables etc). Today, most of those shared objects have been given
their own locks, but nobody has yet gone through the code line by line
in order to check that it is safe to remove the BKL.

Cheers,
 Trond

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Peter Staubach]

Xin Zhao wrote:

>Thanks for your reply. So the only reason is for rpc auditing? If so,
>why not just lock the code that updating the audit information? Now
>the code is:
>
>lock_kernel()
>rpc_execute()
>unlock_kernel().
>
>That means the kernel will be blocked when rpc is executed, which
>could take long time. Even if rpc_execute() won't take very long, this
>implementation still looks inefficient. That's why I am a little
>confused on this point.
>
>Any further thought?
>

I would suggest looking at the semantics of the BKL.  They don't end up
implying what you are suggesting.  The kernel isn't really locked for
the duration of the over the wire RPC.

    Thanx...

       ps

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Xin Zhao]

Thanks for your guys' reply!

Peter, can you point me somewhere I can check the semantics of BKL? In
the past, I remembered BKL does block the kernel. So I am quite
confused now.

Also, I still don't understand why we use lock_kernel instead of some
finer granularity lock. Trond's answer gave me a feeling that this is
simply because the code is not carefully optimized.  :)

-x

On 4/25/06, Peter Staubach <staubach@redhat.com> wrote:
> Xin Zhao wrote:
>
> >Thanks for your reply. So the only reason is for rpc auditing? If so,
> >why not just lock the code that updating the audit information? Now
> >the code is:
> >
> >lock_kernel()
> >rpc_execute()
> >unlock_kernel().
> >
> >That means the kernel will be blocked when rpc is executed, which
> >could take long time. Even if rpc_execute() won't take very long, this
> >implementation still looks inefficient. That's why I am a little
> >confused on this point.
> >
> >Any further thought?
> >
>
> I would suggest looking at the semantics of the BKL.  They don't end up
> implying what you are suggesting.  The kernel isn't really locked for
> the duration of the over the wire RPC.
>
>     Thanx...
>
>        ps
>

Re: question about nfs_execute_read: why do we need to do lock_kernel?

[Trond Myklebust]

On Tue, 2006-04-25 at 11:31 -0400, Xin Zhao wrote:
> Thanks for your guys' reply!
> 
> Peter, can you point me somewhere I can check the semantics of BKL? In
> the past, I remembered BKL does block the kernel. So I am quite
> confused now.

That would have been true >10 years ago, when Alan introduced it.
Nowadays, the BKL is just another type of lock, albeit with very unique
properties. The two main ones being:
        - it can be taken recursively.
        - you can call schedule() while holding it.
                Note however, that upon yielding, the process
                automatically gives up the lock, then retakes it when it
                is rescheduled

> Also, I still don't understand why we use lock_kernel instead of some
> finer granularity lock. Trond's answer gave me a feeling that this is
> simply because the code is not carefully optimized.  :)

Sigh... Removing the BKL is not a trivial thing to do. You have make
absolutely sure that you understand the thread data dependencies that
are protected by that lock. That is why we do it gradually.

Cheers,
  Trond