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=-6.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 3D49BC432BE for ; Mon, 26 Jul 2021 13:11:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1D8A36056C for ; Mon, 26 Jul 2021 13:11:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233115AbhGZMak (ORCPT ); Mon, 26 Jul 2021 08:30:40 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:27427 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231874AbhGZMah (ORCPT ); Mon, 26 Jul 2021 08:30:37 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1627305065; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tPL6SyJfCAcGFknIr89e6g6Y7tOrsxyCmKRCnF/Y8R0=; b=NIBwVE6EPsHiD8SwHPA/Mu5OtfZ0lbgi4oq6m+r2+TV9vhZZhINtVJ6cOMOXxFMF/ivvdU +ifIjIHASGwJ92EZHezojhDlAZ0OuLYSVXgmQpLMkQsYfDif3mAkOF4bxC+U9NWjhu/LxC R12HdjaeesYCqlYJcvgSFDLyBixrjqk= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-141-y8nFsT7AMCW3CBoFVjZcGA-1; Mon, 26 Jul 2021 09:11:04 -0400 X-MC-Unique: y8nFsT7AMCW3CBoFVjZcGA-1 Received: by mail-wm1-f70.google.com with SMTP id j11-20020a05600c410bb02902278758ab90so2677089wmi.9 for ; Mon, 26 Jul 2021 06:11:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:content-transfer-encoding; bh=tPL6SyJfCAcGFknIr89e6g6Y7tOrsxyCmKRCnF/Y8R0=; b=NwdK09NHKc2Bf9KBiMSR5Q8YRnkr7tnYoNEQSSApavBF18H5uMssen+Rc1dns7uYZy CGtoLxnyNfJYO8I05wWL0f930BiZV1YfKp9AzY2njQjRqId7noRI5mYeMqmtQzWVF5hM ez4uWtMooFskDg+Cxrwy/UfW4IF3sYDsHYGmNB+SmaDXdcwAubTl0N0VozXLSinck5Z1 5s0+QsNKIHTqeHE/UUNYFuPBTwGJHH9GmXVI9cYQGtUqmYVmKqdU/f6prn/wG4dn5kBg 4ncAPcTLqBYn7TJjZOZM75JzHpq+3QKSAmJ2swo00XGzoKN1NX4qbJ7EpfAJxoeGKU8t agEQ== X-Gm-Message-State: AOAM533Oe1kklN7FcZBf8kF04DMzB2LasTKqESgeBQF5dye67bvfobdD MixvbR6BoCxkAC6I9BOxy5ISybAYXJlrin7ZtVN4FbAzZ8ZTRV3E87UD4nZdpCAMJvsMqWdiz3w 1ioUOXzkypWbAusop2Fs9vPgxdrE79QGyHpYMcmvA X-Received: by 2002:a1c:2282:: with SMTP id i124mr17092303wmi.166.1627305063117; Mon, 26 Jul 2021 06:11:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyazdqGI0qehD4Mm0gUxh5ITfYFs2JsUFWs3CSE0J7pUjoTGjDY2Mt93MVWMQBRvSg882EFFsTvTDaZEHsMF0g= X-Received: by 2002:a1c:2282:: with SMTP id i124mr17092287wmi.166.1627305062909; Mon, 26 Jul 2021 06:11:02 -0700 (PDT) MIME-Version: 1.0 References: <20210723174131.180813-1-hsiangkao@linux.alibaba.com> <20210725221639.426565-1-agruenba@redhat.com> <20210726110611.459173-1-agruenba@redhat.com> <20210726121702.GA528@lst.de> In-Reply-To: From: Andreas Gruenbacher Date: Mon, 26 Jul 2021 15:10:51 +0200 Message-ID: Subject: Re: [PATCH v7] iomap: make inline data support more flexible To: =?UTF-8?Q?Andreas_Gr=C3=BCnbacher?= , Christoph Hellwig , Andreas Gruenbacher , "Darrick J . Wong" , Matthew Wilcox , Huang Jianan , linux-erofs@lists.ozlabs.org, Linux FS-devel Mailing List , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 26, 2021 at 2:51 PM Gao Xiang wro= te: > Hi Andreas, Christoph, > > On Mon, Jul 26, 2021 at 02:27:12PM +0200, Andreas Gr=C3=BCnbacher wrote: > > Am Mo., 26. Juli 2021 um 14:17 Uhr schrieb Christoph Hellwig : > > > > > > > Subject: iomap: Support tail packing > > > > > > I can't say I like this "tail packing" language here when we have the > > > perfectly fine inline wording. Same for various comments in the actu= al > > > code. > > > > > > > + /* inline and tail-packed data must start page aligned in the= file */ > > > > + if (WARN_ON_ONCE(offset_in_page(iomap->offset))) > > > > + return -EIO; > > > > + if (WARN_ON_ONCE(size > PAGE_SIZE - offset_in_page(iomap->inl= ine_data))) > > > > + return -EIO; > > > > > > Why can't we use iomap_inline_data_size_valid here? > > > > We can now. Gao, can you change that? > > Thank you all taking so much time on this! much appreciated. > > I'm fine to update that. > > > > > > That is how can size be different from iomap->length? > > > > Quoting from my previous reply, > > > > "In the iomap_readpage case (iomap_begin with flags =3D=3D 0), > > iomap->length will be the amount of data up to the end of the inode. > > For tail-packing cases, iomap->length is just the length of tail-packing > inline extent. > > > In the iomap_file_buffered_write case (iomap_begin with flags =3D=3D > > IOMAP_WRITE), iomap->length will be the size of iomap->inline_data. > > (For extending writes, we need to write beyond the current end of > > inode.) So iomap->length isn't all that useful for > > iomap_read_inline_data." > > Ok, now it seems I get your point. For the current gfs2 inline cases: > iomap_write_begin > iomap_write_begin_inline > iomap_read_inline_data > > here, gfs2 passes a buffer instead with "iomap->length", maybe it > could be larger than i_size_read(inode) for gfs2. Is that correct? > > loff_t max_size =3D gfs2_max_stuffed_size(ip); > > iomap->length =3D max_size; > > If that is what gfs2 currently does, I think it makes sense to > temporarily use as this, but IMO, iomap->inline_bufsize is not > iomap->length. These are 2 different concepts. > > > > > > Shouldn't the offset_in_page also go into iomap_inline_data_size_vali= d, > > > which should probably be called iomap_inline_data_valid then? > > > > Hmm, not sure what you mean: iomap_inline_data_size_valid does take > > offset_in_page(iomap->inline_data) into account. > > > > > > if (iomap->type =3D=3D IOMAP_INLINE) { > > > > + int ret =3D iomap_read_inline_data(inode, page, iomap= ); > > > > + return ret ?: PAGE_SIZE; > > > > > The ?: expression without the first leg is really confuing. Especial= ly > > > if a good old if is much more readable here. > > > > I'm sure Gao can change this. > > > > > int ret =3D iomap_read_inline_data(inode, page, iomap= ); > > > > > > if (ret) > > > return ret; > > > return PAGE_SIZE; > > I'm fine to update it if no strong opinion. > > > > > > > > + copied =3D copy_from_iter(iomap_inline_data(iomap, po= s), length, iter); > > > > > > > > > > + copied =3D copy_to_iter(iomap_inline_data(iomap, pos)= , length, iter); > > > > > > Pleae avoid the overly long lines. > > > > I thought people were okay with 80 character long lines? > > Christoph mentioned before as below: > https://lore.kernel.org/linux-fsdevel/YPVe41YqpfGLNsBS@infradead.org/ > > We also need to take the offset into account for the write side. > I guess it would be nice to have a local variable for the inline > address to not duplicate that calculation multiple times. Fair enough, we could add a local variable: void *inline_data =3D iomap_inline_data(iomap, pos); and use that in the copy_from_iter and copy_to_iter. Why not. Thanks, Andreas