Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210316221921.1124955-1-harshadshirwadkar@gmail.com> <CAOQ4uxiD8WGLeSftqL6dOfz_kNp+YSE7qfXYG34Pea4j8G7CxA@mail.gmail.com>
In-Reply-To: <CAOQ4uxiD8WGLeSftqL6dOfz_kNp+YSE7qfXYG34Pea4j8G7CxA@mail.gmail.com>
From:   harshad shirwadkar <harshadshirwadkar@gmail.com>
Date:   Thu, 18 Mar 2021 18:32:13 -0700
Message-ID: <CAD+ocbzMv6SyUUZFnBE0gTnHf8yvMFfq6Dm9rdnLXoUrh7gYkg@mail.gmail.com>
Subject: Re: [PATCH] ext4: add rename whiteout support for fast commit
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Ext4 <linux-ext4@vger.kernel.org>, Theodore Tso <tytso@mit.edu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

So, I'm planning to work on that in the near future, but meanwhile we
can carry this patch so that at least we don't break rename whiteout
after fast commit replays.

Thanks,
Harshad

On Wed, Mar 17, 2021 at 2:33 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Mar 17, 2021 at 12:19 AM Harshad Shirwadkar
> <harshadshirwadkar@gmail.com> wrote:
> >
> > This patch adds rename whiteout support in fast commits. Note that the
>
> My only problem with this change is the subject.
> It sounds like rename whiteout was not possible and now support was added
> and it is now possible. This is not the case.
> The truth is that rename whiteout is supported but broken with fast commits.
> So the subject should reflect that this is a FIX commit, i.e.:
>
> "ext4: fix rename whiteout with fast commit"
>
> And patch should have a Fixes: tag with the commit that added fast commit
> support to rename.
>
> Otherwise, patch has stray newline but the rest looks pretty straightforward
> to me.
>
> Thanks,
> Amir.