linux-kernel - Re: [PATCH 2/2] UBI: Block: Add blk-mq support

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] [day] [month] [year] [list]

Message-ID: <54B19F02.2070506@nod.at>
Date:	Sat, 10 Jan 2015 22:52:02 +0100
From:	Richard Weinberger <richard@....at>
To:	Christoph Hellwig <hch@...radead.org>
CC:	dedekind1@...il.com, dwmw2@...radead.org,
	computersforpeace@...il.com, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org, axboe@...com, tom.leiming@...il.com
Subject: Re: [PATCH 2/2] UBI: Block: Add blk-mq support

Am 10.01.2015 um 19:58 schrieb Christoph Hellwig:
>> +struct ubiblock_pdu {
>> +	struct request *req;
> 
> No need to store the request, you can trivially get at it using
> blk_mq_rq_from_pdu().

Very handy, I was not aware of blk_mq_rq_from_pdu().

>> +	struct ubiblock *dev;
> 
> Why do you need the dev pointer?  You can always trivially get
> it using req->queuedata.

Same here.

>> +static void ubiblock_do_work(struct work_struct *work)
>> +{
>> +	int ret;
>> +	struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
>> +
>> +	ret = ubiblock_read(pdu);
>> +	blk_mq_end_request(pdu->req, ret ?: 0);
> 
> Why not just pass ret as-is?

Obviously a brain fart. :-\

>> +	if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
>> +	    get_capacity(req->rq_disk))
>> +		return BLK_MQ_RQ_QUEUE_ERROR;
> 
> The upper layers take are of this check.

Ok.

>> +	pdu->usgl.list_pos = 0;
>> +	pdu->usgl.page_pos = 0;
> 
> Having a helper to initialize a ubi_sgl would be nicer than having
> to open code it ike here.

Ok.

>> +
>> +	blk_mq_start_request(req);
>> +	ret = blk_rq_map_sg(hctx->queue, req, pdu->usgl.sg);
>> +
>> +	queue_work(dev->wq, &pdu->work);
> 
> Why don't you move these calls into the work queue as well?  The
> queue_rq call would literally just become a queue_work call.

I did not know that I'm allowed to get hctx->queue also via req->q.

> And given that this is a fairly common patter this should allow
> us to refactor / optimize this case a bit later on.

Will send a v2 in a jiffy!

Thanks,
//richard
--
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/