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@xxxxxxxxx> wrote: > > On Wed, Mar 17, 2021 at 12:19 AM Harshad Shirwadkar > <harshadshirwadkar@xxxxxxxxx> 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.
