Re: [PATCH] Change ll_rw_block() calls in JBD

Jan Kara
2006-05-19T01:30:08Z

> Getting better :-)
>
> >+ was_dirty = buffer_dirty(bh);
>
> Why do not we use "buffer_jbddirty()"?
I think Stephen has already explained it... We have a data buffer and
hence we use buffer_dirty()

>
> >+ if (was_dirty && test_set_buffer_locked(bh)) {
> >+ BUFFER_TRACE(bh, "needs blocking lock");
> >+ get_bh(bh);
>
> Why do you need a "get_bh(bh)"?
> "bh" is attached to "jh".
> Can it go away while waiting for the buffer lock?
> ("jh" in on "t_sync_datalist", it cannot go away.)
>
> >+ spin_unlock(&journal->j_list_lock);
> >+ lock_buffer(bh);
> >+ spin_lock(&journal->j_list_lock);
> >+ /* Someone already cleaned up the buffer? Restart. */
> >+ if (!buffer_jbd(bh) || jh->b_jlist != BJ_SyncData) {
>
> Who (else) can take away the journal head, remove our "jh" from the
> synch. data list?
For two of the above comments: Under memory pressure data buffers can
be written out earlier and then released by __journal_try_to_free_buffer()
as they are not dirty any more. The above checks protect us against this.

> >+ else {
> >+ BUFFER_TRACE(bh, "needs writeout, submitting");
> > __journal_temp_unlink_buffer(jh);
> > __journal_file_buffer(jh, commit_transaction,
> > BJ_Locked);
>
> A simple "__journal_file_buffer(...)" could be enough as it includes:
>
> if (jh->b_transaction)
> __journal_temp_unlink_buffer(jh);
Yes, you are right here.

> Would not it be more easy to read like this?
>
> if ((!was_dirty && buffer_locked(bh))
> || (was_dirty && test_clear_buffer_dirty(bh))) {
> BUFFER_TRACE(bh, "needs writeout, submitting");
> __journal_temp_unlink_buffer(jh);
> __journal_file_buffer(jh, commit_transaction,
> BJ_Locked);
> jbd_unlock_bh_state(bh);
> if (was_dirty) {
> get_bh(bh);
> bh->b_end_io = end_buffer_write_sync;
> submit_bh(WRITE, bh);
> }
> }
> else {
>
> BUFFER_TRACE(bh, "writeout complete: unfile");
> __journal_unfile_buffer(jh);
> jbd_unlock_bh_state(bh);
> journal_remove_journal_head(bh);
> if (was_dirty)
> unlock_buffer(bh);
> put_bh(bh);
> }
So you basically mean switching those two branches of if.. OK, maybe.

> As synch. data handling is a compact stuff, cannot it be moved out from
> "journal_commit_transaction()" as e.g. "journal_write_revoke_records()"?
> (Just for a better readability...)
Yes, probably moving it to a new function may improve the readability.
Thanks for suggestions.

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