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

[Jan Kara <jack@suse.cz>]

> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/