linux-kernel - Re: [PATCH V6 2/2] virtio-blk: Add REQ_FLUSH and REQ

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]

Message-ID: <20120807091510.GA2651@lst.de>
Date:	Tue, 7 Aug 2012 11:15:10 +0200
From:	Christoph Hellwig <hch@....de>
To:	Asias He <asias@...hat.com>
Cc:	linux-kernel@...r.kernel.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
	Tejun Heo <tj@...nel.org>, Shaohua Li <shli@...nel.org>,
	"Michael S. Tsirkin" <mst@...hat.com>, kvm@...r.kernel.org,
	virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH V6 2/2] virtio-blk: Add REQ_FLUSH and REQ_FUA support
	to bio path

At least after review is done I really think this patch sopuld be folded
into the previous one.

Some more comments below:

> @@ -58,6 +58,12 @@ struct virtblk_req
>  	struct bio *bio;
>  	struct virtio_blk_outhdr out_hdr;
>  	struct virtio_scsi_inhdr in_hdr;
> +	struct work_struct work;
> +	struct virtio_blk *vblk;

I think using bio->bi_private for the virtio_blk pointer would
be cleaner.


> +	bool is_flush;
> +	bool req_flush;
> +	bool req_data;
> +	bool req_fua;

This could be a bitmap, or better even a single state field.

> +static int virtblk_bio_send_flush(struct virtio_blk *vblk,
> +				  struct virtblk_req *vbr)

> +static int virtblk_bio_send_data(struct virtio_blk *vblk,
> +				 struct virtblk_req *vbr)

These should only get the struct virtblk_req * argument as the virtio_blk
structure is easily derivable from it.

> +static inline void virtblk_bio_done_flush(struct virtio_blk *vblk,
> +					  struct virtblk_req *vbr)
>  {
> +	if (vbr->req_data) {
> +		/* Send out the actual write data */
> +		struct virtblk_req *_vbr;
> +		_vbr = virtblk_alloc_req(vblk, GFP_NOIO);
> +		if (!_vbr) {
> +			bio_endio(vbr->bio, -ENOMEM);
> +			goto out;
> +		}
> +		_vbr->req_fua = vbr->req_fua;
> +		_vbr->bio = vbr->bio;
> +		_vbr->vblk = vblk;
> +		INIT_WORK(&_vbr->work, virtblk_bio_send_data_work);
> +		queue_work(virtblk_wq, &_vbr->work);

The _vbr naming isn't too nice.  Also can you explain why the original
request can't be reused in a comment here?

Also if using a state variable I think the whole code would be
a bit cleaner if the bio_done helpers are combined.

> -	if (writeback && !use_bio)
> +	if (writeback)
>  		blk_queue_flush(vblk->disk->queue, REQ_FLUSH);

Shouldn't this be REQ_FLUSH | REQ_FUA for the bio case?

Btw, did you verify that flushes really work correctly for all cases
using tracing in qemu?

--
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/