From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC1A0C4646B for ; Wed, 26 Jun 2019 06:40:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id ACB78208CB for ; Wed, 26 Jun 2019 06:40:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726462AbfFZGka (ORCPT ); Wed, 26 Jun 2019 02:40:30 -0400 Received: from verein.lst.de ([213.95.11.211]:40546 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725876AbfFZGk3 (ORCPT ); Wed, 26 Jun 2019 02:40:29 -0400 Received: by newverein.lst.de (Postfix, from userid 2407) id AB0EC68B05; Wed, 26 Jun 2019 08:39:57 +0200 (CEST) Date: Wed, 26 Jun 2019 08:39:57 +0200 From: Christoph Hellwig To: Goldwyn Rodrigues Cc: Christoph Hellwig , linux-btrfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, darrick.wong@oracle.com, david@fromorbit.com Subject: Re: [PATCH 1/6] iomap: Use a IOMAP_COW/srcmap for a read-modify-write I/O Message-ID: <20190626063957.GA24201@lst.de> References: <20190621192828.28900-1-rgoldwyn@suse.de> <20190621192828.28900-2-rgoldwyn@suse.de> <20190624070734.GB3675@lst.de> <20190625191442.m27cwx5o6jtu2qch@fiona> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190625191442.m27cwx5o6jtu2qch@fiona> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org On Tue, Jun 25, 2019 at 02:14:42PM -0500, Goldwyn Rodrigues wrote: > > I can't say I'm a huge fan of this two iomaps in one method call > > approach. I always though two separate iomap iterations would be nicer, > > but compared to that even the older hack with just the additional > > src_addr seems a little better. > > I am just expanding on your idea of using multiple iterations for the Cow case > in the hope we can come out of a good design: > > 1. iomap_file_buffered_write calls iomap_apply with IOMAP_WRITE flag. > which calls iomap_begin() for the respective filesystem. > 2. btrfs_iomap_begin() sets up iomap->type as IOMAP_COW and fills iomap > struct with read addr information. > 3. iomap_apply() conditionally for IOMAP_COW calls do_cow(new function) > and calls ops->iomap_begin() with flag IOMAP_COW_READ_DONE(new flag). > 4. btrfs_iomap_begin() fills up iomap structure with write information. > > Step 3 seems out of place because iomap_apply should be iomap.type agnostic. > Right? > Should we be adding another flag IOMAP_COW_DONE, just to figure out that > this is the "real" write for iomap_begin to fill iomap? > > If this is not how you imagined, could you elaborate on the dual iteration > sequence? Here are my thoughts from dealing with this from a while ago, all XFS based of course. If iomap_file_buffered_write is called on a page that is inside a COW extent we have the following options: a) the page is updatodate or entirely overwritten. We cn just allocate new COW blocks and return them, and we are done b) the page is not/partially uptodate and not entirely overwritten. The latter case is the interesting one. My thought was that iff the IOMAP_F_SHARED flag is set __iomap_write_begin / iomap_read_page_sync will then have to retreive the source information in some form. My original plan was to just do a nested iomap_apply call, which would need a special nested flag to not duplicate any locking the file system might be holding between ->iomap_begin and ->iomap_end. The upside here is that there is no additional overhead for the non-COW path and the architecture looks relatively clean. The downside is that at least for XFS we usually have to look up the source information anyway before allocating the COW destination extent, so we'd have to cache that information somewhere or redo it, which would be rather pointless. At that point the idea of a srcaddr in the iomap becomes interesting again - while it looks a little ugly from the architectural POV it actually ends up having very practical benefits. > > > -- > Goldwyn ---end quoted text---