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

[Zoltan Menyhart]

I have got a concern about this code fragment:

> +		if (buffer_dirty(bh)) {
> +			if (test_set_buffer_locked(bh)) {
> +				...
> +				get_bh(bh);
> +				spin_unlock(&journal->j_list_lock);
> +				/* Submit all accumulated buffers to prevent
> +				 * possible deadlocks */
> +				submit_buffers(bufs, wbuf);
> +				bufs = 0;
> +				lock_buffer(bh);
> +				spin_lock(&journal->j_list_lock);
> +				/* Someone already cleaned up the buffer? */
> +				if (!buffer_jbd(bh)
> +					|| jh->b_jlist != BJ_SyncData) {
> +					unlock_buffer(bh);
> +					BUFFER_TRACE(bh, "already cleaned up");
> +					put_bh(bh);
> +					continue;
> +				}
> +				put_bh(bh);
> +			}
> +			if (test_clear_buffer_dirty(bh)) {
> +				BUFFER_TRACE(bh, "needs writeout, submitting");
> +				get_bh(bh);
> +				wbuf[bufs++] = bh;
> +				if (bufs == journal->j_wbufsize) {
> +					spin_unlock(&journal->j_list_lock);
> +					/* Writeout will be done at the
> +					 * beginning of the loop */
> +					goto write_out_data;
> +				}
> +			}

We lock up to "->j_wbufsize" buffers, one after the others.

Originally, we toke a buffer, we did something to it, and we released it.
When we wanted two of them and the second one was not available, we
released the first one, too, in order to avoid dead-locks.

Keeping a couple of buffer locked for an unpredictably long time...
(Here you keep N-1 buffer locked while you are waiting for the Nth one.)
And not imposing / respecting any locking order...

The original code did not lock the buffers while it was composing the
list of buffers. "ll_rw_block()" locked one by one the buffers
and submitted them to the BIO. These buffers got eventually unlocked,
possibly before the some last buffers got locked by "ll_rw_block()".

This change implies an important difference in locking behavior.

Regards,

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