ll_rw_block audit

ll_rw_block audit

[Christoph Hellwig]

I've just sent a patch to fsdevel to remove the SWRITE* flags and add
a new helper to write buffers using proper locking, which tricked me
into an audit of all ll_rw_block users.  There's not a lot left now,
so it might be time to kill them.  For writes we only have 6 users left:

fs/buffer.c:		ll_rw_block(WRITE, 1, &bh);
fs/hfs/hfs_fs.h:	ll_rw_block(WRITE, 1, &bh);
fs/reiserfs/journal.c:	ll_rw_block(WRITE, 1, &bh);
fs/reiserfs/journal.c:	ll_rw_block(WRITE, 1, &tbh);

plus two odd commented out ones in jbd/jbd2:

fs/jbd/recovery.c:			/* ll_rw_block(WRITE, 1, &nbh); */
fs/jbd2/recovery.c:			/* ll_rw_block(WRITE, 1, &nbh); */

The one in hfs is obviously a broken attempt to implement
sync_dirty_buffer, I'll send a patch to fix it.

That leaves:

  - write_boundary_block.

	This one is only used by __mpage_writepage and is a data
	integrity operation if WB_SYNC_ALL is set.  It should probably
	use write_dirty_buffer at least for that case.  Is it worth
	keeping the trylock for WB_SYNC_NONE writes?
	Btw, what's the reason this function is in buffer.c instead
	of mpage.c?

 - write_ordered_buffers and flush_commit_list in reiserfs.  Both of
   these look like data integrity operations to me, so using
   write_dirty_buffer seems in order.

For the read side we have 38 users. Most of them wait on the buffer
in some more or less broken form.  These should be replaced with the
bh_uptodate_or_lock/bh_submit_read combination, or possibly a helper
combining the two.  There's also a lot of places that look like
broken reimplementations of __bread_slow.

Re: ll_rw_block audit

[Jan Kara]

On Wed 23-06-10 12:43:25, Christoph Hellwig wrote:
> I've just sent a patch to fsdevel to remove the SWRITE* flags and add
> a new helper to write buffers using proper locking, which tricked me
> into an audit of all ll_rw_block users.  There's not a lot left now,
> so it might be time to kill them.  For writes we only have 6 users left:
> 
> fs/buffer.c:		ll_rw_block(WRITE, 1, &bh);
> fs/hfs/hfs_fs.h:	ll_rw_block(WRITE, 1, &bh);
> fs/reiserfs/journal.c:	ll_rw_block(WRITE, 1, &bh);
> fs/reiserfs/journal.c:	ll_rw_block(WRITE, 1, &tbh);
> 
> plus two odd commented out ones in jbd/jbd2:
> 
> fs/jbd/recovery.c:			/* ll_rw_block(WRITE, 1, &nbh); */
> fs/jbd2/recovery.c:			/* ll_rw_block(WRITE, 1, &nbh); */
  Just nuke these. We mark the buffer dirty shortly before this and
there's no reason to send replayed buffer to disk just now...

> The one in hfs is obviously a broken attempt to implement
> sync_dirty_buffer, I'll send a patch to fix it.
> 
> That leaves:
> 
>   - write_boundary_block.
> 
> 	This one is only used by __mpage_writepage and is a data
> 	integrity operation if WB_SYNC_ALL is set.  It should probably
> 	use write_dirty_buffer at least for that case.  Is it worth
> 	keeping the trylock for WB_SYNC_NONE writes?
  I don't think it must be a data integrity write in WB_SYNC_ALL mode.
All the "buffer_boundary" logic is just an optimization used by some
filesystems (ext2, ext3, gfs2) to get nicely linear IO when data blocks
are intermixed with indirect blocks. In fact, noone even guarantees that
the block written/read by buffer_boundary logic belongs to the file...
Data integrity guarantees are achieved by different means
(either by sync_mapping_buffer() in case of ext2, or by journal commits
in case of ext3, and probably gfs2).

> 	Btw, what's the reason this function is in buffer.c instead
> 	of mpage.c?
  Dunno...
  
>  - write_ordered_buffers and flush_commit_list in reiserfs.  Both of
>    these look like data integrity operations to me, so using
>    write_dirty_buffer seems in order.
  Yes, they are data integrity writes. The first one is writing
data=ordered mode buffer, the second one is writing metadata block to a
journal.

> For the read side we have 38 users. Most of them wait on the buffer
> in some more or less broken form.  These should be replaced with the
> bh_uptodate_or_lock/bh_submit_read combination, or possibly a helper
> combining the two.  There's also a lot of places that look like
> broken reimplementations of __bread_slow.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR