[QUESTION] Buffer locking and similar

[QUESTION] Buffer locking and similar

[Jan Kara]

  Hello,

  I have a simple question: What is exactly the meaning of buffer being
locked (by lock_buffer() or set_buffer_locked())? Because some places -
such as ll_rw_block() suppose that when the buffer is locked it was
already sent for IO (and hence skip the buffer). But other places
violate this assumption and lock_buffer() without submitting it for IO.
What is supposed to be guarded by the buffer lock? Specifically
changes of data in a buffer or marking a buffer dirty does not seem to
need the buffer lock (only if the buffer is uptodate?).
  The final question: should not JBD wait for a buffer lock before it
files a buffer to some list to make sure that a potential write-out of
the buffer is finished? For example do_get_write_access() does not do
this and seems to be racy against __flush_batch().
  I'm willing to make a patch fixing these issues but first I need to be
sure what is actually supposed to be the correct behaviour ;).

					Thanks for an answer
								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [QUESTION] Buffer locking and similar

[Andrew Morton]

Jan Kara <jack@suse.cz> wrote:
>
>   Hello,
> 
>   I have a simple question: What is exactly the meaning of buffer being
> locked (by lock_buffer() or set_buffer_locked())?

Good question.   Lots of things.

- Provides exclusion against a thread bringing the buffer uptodate

- Pins the buffer and its page

- Indicates that the buffer is under I/O

- Probably lots of other ad-hoc stuff ;(

> Because some places -
> such as ll_rw_block() suppose that when the buffer is locked it was
> already sent for IO (and hence skip the buffer).

Yes.  Basically it is illegal to unlock a non-uptodate buffer.  IOW: if you
locked a non-uptodate buffer, ll_rw_block() will assume that you locked it
for the purpose of bringing it uptodate, either via a read or a memset.

> But other places
> violate this assumption and lock_buffer() without submitting it for IO.

If it was a non-uptodate buffer then that's bad.  Perhaps we should have a
diagnostic check for a non-uptodate buffer in unlock_buffer().

> What is supposed to be guarded by the buffer lock? Specifically
> changes of data in a buffer or marking a buffer dirty does not seem to
> need the buffer lock (only if the buffer is uptodate?).

If it's non-uptodate then yes, we need lock_buffer exclusion while bringing
it uptodate.

If the buffer was already uptodate then any modification of its data is a
*dirtying* operattion, not a *bringing uptodate* operation.  That's quite a
different thing, and dirtying doesn't require lock_buffer().  Because we
mark the buffer dirty _after_ changing its contents.

>   The final question: should not JBD wait for a buffer lock before it
> files a buffer to some list to make sure that a potential write-out of
> the buffer is finished?

Maybe.  Would need to see specifics and then go into deep-think mode.

> For example do_get_write_access() does not do
> this and seems to be racy against __flush_batch().

do_get_write_access() does lock_buffer().  Can you be more specific?

Re: [QUESTION] Buffer locking and similar

[Jan Kara]

  Hello,

  sorry for a late response but I was not reading emails for a week.

> Jan Kara <jack@suse.cz> wrote:
> >
> >   Hello,
> > 
> >   I have a simple question: What is exactly the meaning of buffer being
> > locked (by lock_buffer() or set_buffer_locked())?
> 
> Good question.   Lots of things.
> 
> - Provides exclusion against a thread bringing the buffer uptodate
  Umm, I'm not sure about this one - some buffers get set uptodate
while only holding the page lock (e.g. in block_read_full_page()). But
they seem to be freshly created buffers so it is probably OK in this
case.

> - Pins the buffer and its page
> 
> - Indicates that the buffer is under I/O
> 
> - Probably lots of other ad-hoc stuff ;(
> 
> > Because some places -
> > such as ll_rw_block() suppose that when the buffer is locked it was
> > already sent for IO (and hence skip the buffer).
> 
> Yes.  Basically it is illegal to unlock a non-uptodate buffer.  IOW: if you
> locked a non-uptodate buffer, ll_rw_block() will assume that you locked it
> for the purpose of bringing it uptodate, either via a read or a memset.
  Actually the situation I'm worried more about is when we are writing
out. For journaling assumptions we need to be sure that if we submit a
buffer for IO it really hits the disk. But ll_rw_blk() silently skips
the buffer if it is locked. So I guess we should not use ll_rw_blk() for
writing in the journaling code as unlocking uptodate but *dirty* buffer is
perfectly OK from what you say.

> > But other places
> > violate this assumption and lock_buffer() without submitting it for IO.
> 
> If it was a non-uptodate buffer then that's bad.  Perhaps we should have a
> diagnostic check for a non-uptodate buffer in unlock_buffer().
  I'll add it and try to run the kernel :).

> > What is supposed to be guarded by the buffer lock? Specifically
> > changes of data in a buffer or marking a buffer dirty does not seem to
> > need the buffer lock (only if the buffer is uptodate?).
> 
> If it's non-uptodate then yes, we need lock_buffer exclusion while bringing
> it uptodate.
> 
> If the buffer was already uptodate then any modification of its data is a
> *dirtying* operattion, not a *bringing uptodate* operation.  That's quite a
> different thing, and dirtying doesn't require lock_buffer().  Because we
> mark the buffer dirty _after_ changing its contents.
  OK, I see the point. Thanks for explanation.

> >   The final question: should not JBD wait for a buffer lock before it
> > files a buffer to some list to make sure that a potential write-out of
> > the buffer is finished?
> 
> Maybe.  Would need to see specifics and then go into deep-think mode.
  See below...

> > For example do_get_write_access() does not do
> > this and seems to be racy against __flush_batch().
> 
> do_get_write_access() does lock_buffer().  Can you be more specific?
  Consider the below race:
    Proc 1                               Proc 2

     __flush_batch()
       ll_rw_block()
                                        do_get_write_access()
                                           do some checks under lock_buffer()
                                           unlock_buffer
    test_set_buffer_locked()
    test_clear_buffer_dirty()
                                           __journal_file_buffer()
                                        change the data
    submit_bh()

  In this case we have made a mistake and attached the buffer to the
transaction even though it is on the way to a disk. Hence some unexpected
data might be written.

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs