linux-kernel - Re: [patch,v2] block: fix flush machinery for stacking drivers with differring flush flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 13 Aug 2011 15:29:10 +0200
From:	Tejun Heo <tj@...nel.org>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	Jens Axboe <jaxboe@...ionio.com>, shaohua <shli@...nel.org>,
	Vivek Goyal <vgoyal@...ibm.com>, dm-devel@...hat.com,
	linux-kernel@...r.kernel.org, Mike Snitzer <msnitzer@...hat.com>
Subject: Re: [patch,v2] block: fix flush machinery for stacking drivers with
 differring flush flags

Hello,

On Fri, Aug 12, 2011 at 03:07:51PM -0400, Jeff Moyer wrote:
> Changes from v1->v2:
> - Moved the detection of empty flush requests into blk_insert_flush.
> - Got rid of REQ_FLUSH_SEQ in the CLONE_FLAGS.

Heh yeah, this looks pretty good to me. :)

> @@ -312,6 +309,19 @@ void blk_insert_flush(struct request *rq)
>  		rq->cmd_flags &= ~REQ_FUA;
>  
>  	/*
> +	 * An empty flush handed down from a stacking driver may
> +	 * translate into nothing if the underlying device does not
> +	 * advertise a write-back cache.  In this case, simply
> +	 * complete the request.
> +	 */
> +	if (!policy && !blk_rq_bytes(rq)) {
> +		__blk_end_bidi_request(rq, 0, 0, 0);
> +		return;
> +	}

Hmmm... doesn't !policy imply !blk_rq_bytes() with your change just
merged to Jens' tree?

> @@ -319,6 +329,7 @@ void blk_insert_flush(struct request *rq)
>  	if ((policy & REQ_FSEQ_DATA) &&
>  	    !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>  		list_add_tail(&rq->queuelist, &q->queue_head);
> +		blk_run_queue_async(q);
>  		return;
>  	}

In the other message, you said,

> Well, the only time we need to run the queue is when the request has
> data, has REQ_FUA set, and the underlying queue's flush flags contain
> only REQ_FUA.  In code:
> 
> if (rq->cmd_flags & REQ_FUA && q->flush_flags == REQ_FUA)
>        blk_run_queue_async(q);

But this can't happen because a queue can't have REQ_FUA without
REQ_FLUSH (it doesn't make any sense).  blk_queue_flush() will trigger
WARN_ON_ONCE() and turn off REQ_FUA in such cases.

That said, it's kinda unclear who should be responsible for kicking
the queue.  __elv_add_request() does it for some but not all.
__make_request() always activates the queue which sometimes ends up
doing it again after __elv_add_request().  I think kicking the queue
after short circuit insert probably is the right thing to do.

Thank you.

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