> 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 <[email protected]>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
