Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752264AbdHJL0I (ORCPT ); Thu, 10 Aug 2017 07:26:08 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:34071 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751418AbdHJL0G (ORCPT ); Thu, 10 Aug 2017 07:26:06 -0400 Date: Thu, 10 Aug 2017 04:26:03 -0700 From: Christoph Hellwig To: Ming Lei Cc: Jens Axboe , Christoph Hellwig , Huang Ying , Andrew Morton , Alexander Viro , linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-bcache@vger.kernel.org Subject: Re: [PATCH v3 07/49] bcache: comment on direct access to bvec table Message-ID: <20170810112603.GD20308@infradead.org> References: <20170808084548.18963-1-ming.lei@redhat.com> <20170808084548.18963-8-ming.lei@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170808084548.18963-8-ming.lei@redhat.com> User-Agent: Mutt/1.8.3 (2017-05-23) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1443 Lines: 37 I think all this bcache code needs bigger attention. For one bio_alloc_pages is only used in bcache, so we should move it in there. Second the way bio_alloc_pages is currently written looks potentially dangerous for multi-page biovecs, so we should think about a better calling convention. The way bcache seems to generally use it is by allocating a bio, then calling bch_bio_map on it and then calling bio_alloc_pages. I think it just needs a new bio_alloc_pages calling convention that passes the size to be allocated and stop looking into the segment count. Second bch_bio_map isn't something we should be doing in a driver, it should be rewritten using bio_add_page. > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c > index 866dcf78ff8e..3da595ae565b 100644 > --- a/drivers/md/bcache/btree.c > +++ b/drivers/md/bcache/btree.c > @@ -431,6 +431,7 @@ static void do_btree_node_write(struct btree *b) > > continue_at(cl, btree_node_write_done, NULL); > } else { > + /* No harm for multipage bvec since the new is just allocated */ > b->bio->bi_vcnt = 0; This should go away - bio_alloc_pages or it's replacement should not modify bi_vcnt on failure. > + /* single page bio, safe for multipage bvec */ > dc->sb_bio.bi_io_vec[0].bv_page = sb_page; needs to use bio_add_page. > + /* single page bio, safe for multipage bvec */ > ca->sb_bio.bi_io_vec[0].bv_page = sb_page; needs to use bio_add_page.