From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Date: Mon, 24 Feb 2020 13:43:47 -0800 Subject: [Cluster-devel] [PATCH v7 14/24] btrfs: Convert from readpages to readahead In-Reply-To: <20200220155727.GA32232@infradead.org> References: <20200219210103.32400-1-willy@infradead.org> <20200219210103.32400-15-willy@infradead.org> <20200220134849.GV24185@bombadil.infradead.org> <20200220154658.GA19577@infradead.org> <20200220155452.GX24185@bombadil.infradead.org> <20200220155727.GA32232@infradead.org> Message-ID: <20200224214347.GH13895@infradead.org> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Thu, Feb 20, 2020 at 07:57:27AM -0800, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > > > > btrfs: Convert from readpages to readahead > > > > > > > > Implement the new readahead method in btrfs. Add a readahead_page_batch() > > > > to optimise fetching a batch of pages at once. > > > > > > Shouldn't this readahead_page_batch heper go into a separate patch so > > > that it clearly stands out? > > > > I'll move it into 'Put readahead pages in cache earlier' for v8 (the > > same patch where we add readahead_page()) > > One argument for keeping it in a patch of its own is that btrfs appears > to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap, > so it might go away pretty soon and we could just revert the commit. > > But this starts to get into really minor details, so I'll shut up now :) So looking at this again I have another comment and a question. First I think the implicit ARRAY_SIZE in readahead_page_batch is highly dangerous, as it will do the wrong thing when passing a pointer or function argument. Second I wonder ?f it would be worth to also switch to a batched operation in iomap if the xarray overhead is high enough. That should be pretty trivial, but we don't really need to do it in this series.