Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752932AbbGULNE (ORCPT ); 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 ); 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 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==?= , CC: , , , David Vrabel , "Boris Ostrovsky" , 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: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2387 Lines: 81 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 >> Cc: Konrad Rzeszutek Wilk >> Cc: Roger Pau Monné >> Cc: Boris Ostrovsky >> Cc: David Vrabel > > 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é 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/