From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752932AbbGULNE (ORCPT <rfc822;w@1wt.eu>);
	Tue, 21 Jul 2015 07:13:04 -0400
Received: from smtp02.citrix.com ([66.165.176.63]:53989 "EHLO
	SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750779AbbGULNB (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 21 Jul 2015 07:13:01 -0400
X-IronPort-AV: E=Sophos;i="5.15,515,1432598400"; 
   d="scan'208";a="286017386"
Message-ID: <55AE290C.8010103@citrix.com>
Date: Tue, 21 Jul 2015 12:12:12 +0100
From: Julien Grall <julien.grall@citrix.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0
MIME-Version: 1.0
To: =?UTF-8?B?Um9nZXIgUGF1IE1vbm7DqQ==?= <roger.pau@citrix.com>,
        <xen-devel@lists.xenproject.org>
CC: <ian.campbell@citrix.com>, <stefano.stabellini@eu.citrix.com>,
        <linux-kernel@vger.kernel.org>, David Vrabel <david.vrabel@citrix.com>,
        "Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [Xen-devel] [PATCH v2 05/20] block/xen-blkfront: Split blkif_queue_request
 in 2
References: <1436474552-31789-1-git-send-email-julien.grall@citrix.com>
	<1436474552-31789-6-git-send-email-julien.grall@citrix.com>
 <55AE16EC.2020204@citrix.com>
In-Reply-To: <55AE16EC.2020204@citrix.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
X-DLP: MIA1
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Roger,

On 21/07/15 10:54, Roger Pau Monné wrote:
> El 09/07/15 a les 22.42, Julien Grall ha escrit:
>> Currently, blkif_queue_request has 2 distinct execution path:
>>     - Send a discard request
>>     - Send a read/write request
>>
>> The function is also allocating grants to use for generating the
>> request. Although, this is only used for read/write request.
>>
>> Rather than having a function with 2 distinct execution path, separate
>> the function in 2. This will also remove one level of tabulation.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: David Vrabel <david.vrabel@citrix.com>
> 
> Patch looks fine, although with so much indentation changes it's kind of
> hard to review.

I wasn't sure how to make this patch more easy to review and it seems
like diff is getting confused.

It's mostly removing one indentation layer (the if (req->cmd_flags ...))
and move the discard code in a separate function.

> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thank you.

> Just one minor change below.
> 
> [...]
> 
>> @@ -595,6 +603,24 @@ static int blkif_queue_request(struct request *req)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Generate a Xen blkfront IO request from a blk layer request.  Reads
>> + * and writes are handled as expected.
>> + *
>> + * @req: a request struct
>> + */
>> +static int blkif_queue_request(struct request *req)
>> +{
>> +	struct blkfront_info *info = req->rq_disk->private_data;
>> +
>> +	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
>> +		return 1;
>> +
>> +	if (unlikely(req->cmd_flags & (REQ_DISCARD | REQ_SECURE)))
>> +		return blkif_queue_discard_req(req);
>> +	else
>> +		return blkif_queue_rw_req(req);
> 
> There's no need for the else clause.

I find it more readable and obvious to understand than:

if ( ... )
  return
return;

when there is only one line in the else. IIRC, the resulting assembly
will be the same.

Anyway, I can drop the else if you really want.

Regards,

-- 
Julien Grall