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=-8.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 EB0B8C433E6 for ; Fri, 19 Mar 2021 08:37:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C268D64E20 for ; Fri, 19 Mar 2021 08:37:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234293AbhCSIgy (ORCPT ); Fri, 19 Mar 2021 04:36:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35610 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234360AbhCSIgj (ORCPT ); Fri, 19 Mar 2021 04:36:39 -0400 Received: from mail-vk1-xa33.google.com (mail-vk1-xa33.google.com [IPv6:2607:f8b0:4864:20::a33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7C82DC06175F for ; Fri, 19 Mar 2021 01:36:39 -0700 (PDT) Received: by mail-vk1-xa33.google.com with SMTP id o4so1904617vka.12 for ; Fri, 19 Mar 2021 01:36:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=V4zhu0iRCwggJtth86gVHoOF9LsK67CiVV+65APy0lw=; b=NgCrMxdUv4Dd24ZJIvn8txNnJehnCB0Ef7rmPJEieE+/Pctto1uAw6wc8B+cAtlWPK VogFl9h1lhnO/pQRfQNdNah50HLP2xzpEuCnE3YgrDtMJwGvVcwuif9zFRL72kwm9DBt MWvTGTqzBo/n60C6tmFnyjn7KQ95cqhO2fEH4= 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:cc; bh=V4zhu0iRCwggJtth86gVHoOF9LsK67CiVV+65APy0lw=; b=HTMF2+EUvfqkxgAQt//DYlJXl41ps06rj3q7uwpVG9FYRqsANMfyy5BBizavfsNbfe COM1nFiLzlaa8MlblI8fsxI1zAanXvRI54AbMecaLA7IA2ZJklHJ0+wqgN9Dh1cN2yN2 hXXamNW8kt7qi0RJH8QxEju6ff0hannZG6O4DnQZvsvjvYhtCJl53kzyLEa6Y4nUnvfB ohEb8XCzobmzyh2L7TJetLJdI8FKhjHl63OBDnu7u+LcYvEsvnj+jr3YVgmoY4KBvxar wUUfyU8VHkpaAR4f6NONQ03MV+FIdECjs74Q93TJEQUXN51lWDUP8dMX1RcG+deBLRTI 9Z1w== X-Gm-Message-State: AOAM530WFJNJmtWKNv6IMtD/orpyUr+oiRMOvK4vscHtmZUBWhAspDdS 72xp8vGuX/4Tl8pglLLMAol3n5oq0V1FbQOFO9aOog== X-Google-Smtp-Source: ABdhPJyOcUk9BCpUQ0OmCXVP5bM2ZRg/vsU1onqAIu9UwyE5TCXFqJUpY3TQHcZ9BOa0vAr40gKFZ8brFU5MB+38h8k= X-Received: by 2002:a05:6122:54:: with SMTP id q20mr1666620vkn.3.1616142998580; Fri, 19 Mar 2021 01:36:38 -0700 (PDT) MIME-Version: 1.0 References: <20210316221921.1124955-1-harshadshirwadkar@gmail.com> In-Reply-To: From: Miklos Szeredi Date: Fri, 19 Mar 2021 09:36:27 +0100 Message-ID: Subject: Re: [PATCH] ext4: add rename whiteout support for fast commit To: Amir Goldstein Cc: harshad shirwadkar , Ext4 , Theodore Tso , overlayfs Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-unionfs@vger.kernel.org On Fri, Mar 19, 2021 at 6:52 AM Amir Goldstein wrote: > > [adding overlayfs list] > > On Fri, Mar 19, 2021 at 3:32 AM harshad shirwadkar > wrote: > > > > Thanks for the review Amir. > > > > Sure changing the subject makes sense. > > > > Also, on further discussions on Ext4 conference call, we also thought > > that with this patch, overlayfs customers would not benefit from fast > > commits much if they call renames often. So, in order to really make > > rename whiteout a fast commit compatible operation, we probably would > > need to add support in fast commit to replay a char device creation > > event (since whiteout object is a char device). That would imply, we > > would need to do careful versioning and would need to burn an on-disk > > feature flag. > > > > An alternative to this would be to have a static whiteout object with > > irrelevant nlink count and to have every rename point to that object > > instead. Based on how we decide to implement that, at max only the > > first rename operation would be fast commit incompatible since that's > > when this object would get created. All the further operations would > > be fast commit compatible. The big benefit of this approach is that > > this way we don't have to add support for char device creation in fast > > commit replay code and thus we don't have to worry about versioning. > > > > I'm glad to hear that, Harshad. > > Please note that creating a static whiteout object on-disk is one possible > implementation option. Not creating any object on-disk may be even better. I don't really get it. What's the advantage of not having an object? Readdir returning DT_WHT internally might be nice, but I'd be careful with exporting that to userspace, since it's likely to cause more problems that it solves. And on the stat(2) interface adding S_IFWHT or even worse: ENOENT are really out of the question due to backward incompatibility with almost every application. > One other challenge is how to handle users trying to make operations > on the upper layer directly (migrating images etc). > As long as the tools still observe the whiteout as a chadev (with stat(2)) > then export and import should work fine (creating a real chardev on import). Right. Can't mkfs.ext4 just create the static object? That sounds to me like the simplest approach. Thanks, Miklos Thanks, Miklos > > I had suggested the static object approach because it should be pretty > simple to implement and add e2fsprogs support for. > > However, if we look at the requirements for RENAME_WHITEOUT, > the resulting directory entry does not actually need to point to any > object on-disk at all. > > An alternative implementation would be to create a directory entry > with {d_ino = 0, d_type = DT_WHT}. Lookup of this entry will return > a reference to a singleton read-only ext4 whiteout inode object, which > does not reside on disk, so fast commit is irrelevant in that sense. > i_nlink should be handled carefully, but that should be easier from > doing that for a static on-disk object. > > I am not sure how userland tools handle DT_WHT, but I see that > other filesystems can emit this value in theory and man rename(2) > claims that BSD uses DT_WHT, so the common tools should be > able to handle it. > > As far as overlayfs is concerned: > 1. ovl_lookup() will find an IS_WHITEOUT() inode as usual > 2. ovl_dir_read_merged() will need this small patch (below) and will > not access the inode object at all > 3. At first, ovl_whiteout() -> vfs_whiteout() can still create a usual chardev > 4. Later, we can initiate the overlayfs instance singleton whiteout > reference in ovl_check_rename_whiteout() and ovl_whiteout() will > never get -EMLINK when linking this whiteout object > > If there are tools that try to change inode permissions recursively on the > upper layer (?) there may be a problem with those read-only whiteouts > although the permission of a whiteout is a moot concept. > > Thanks, > Amir. > > --- a/fs/overlayfs/readdir.c > +++ b/fs/overlayfs/readdir.c > @@ -161,7 +161,7 @@ static struct ovl_cache_entry > *ovl_cache_entry_new(struct ovl_readdir_data *rdd, > if (ovl_calc_d_ino(rdd, p)) > p->ino = 0; > p->is_upper = rdd->is_upper; > - p->is_whiteout = false; > + p->is_whiteout = (d_type == DT_WHT); > > if (d_type == DT_CHR) { > p->next_maybe_whiteout = rdd->first_maybe_whiteout;