From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932858AbcJZMLU (ORCPT ); Wed, 26 Oct 2016 08:11:20 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33543 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932550AbcJZMLO (ORCPT ); Wed, 26 Oct 2016 08:11:14 -0400 MIME-Version: 1.0 In-Reply-To: References: <1477380887-21333-1-git-send-email-mszeredi@redhat.com> <1477380887-21333-4-git-send-email-mszeredi@redhat.com> From: Amir Goldstein Date: Wed, 26 Oct 2016 15:11:11 +0300 Message-ID: Subject: Re: [PATCH 3/3] ovl: redirect on rename-dir To: Miklos Szeredi Cc: Miklos Szeredi , "linux-unionfs@vger.kernel.org" , Guillem Jover , Raphael Hertzog , linux-fsdevel , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 26, 2016 at 2:26 PM, Miklos Szeredi wrote: > On Tue, Oct 25, 2016 at 2:49 PM, Amir Goldstein wrote: > >>> @@ -880,31 +913,34 @@ static int ovl_rename(struct inode *olddir, struct dentry *old, >>> if (WARN_ON(olddentry->d_inode == newdentry->d_inode)) >>> goto out_dput; >>> >>> - if (is_dir && !old_opaque && ovl_lower_positive(new)) { >>> - err = ovl_set_opaque(olddentry); >>> - if (err) >>> - goto out_dput; >>> - ovl_dentry_set_opaque(old, true); >>> + if (is_dir) { >>> + if (ovl_type_merge_or_lower(old)) { >>> + err = ovl_set_redirect(old); >> >> There is a fair chance of getting ENOSPC/EDQUOT here and confuse user space. >> Would it be better to convert these non fatal errors with EXDEV, so >> user space will >> gracefully fallback to recursive rename/clone/copy? > > Recursive copy up will surely consume more space than an xattr? Surely. But that is not what I meant. In Ext4, for example, the total size of extended attributes for an inode is limited to a single block, so you can get ENOSPC with alot of free space in the file system. You can also get ERANGE if the redirect path length > 256. I'm just saying that instead of failing the move with ERANGE or uncalled for ENOSPC, it is better for user space to get EXDEV, from which there is a sane fallback. BTW, mv (from linux-utils) falls back from -EXDEV to recursive move, which is quite efficient with clone up. > >>> @@ -162,6 +223,23 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, >>> stack[ctr].dentry = this; >>> stack[ctr].mnt = lowerpath.mnt; >>> ctr++; >>> + >>> + if (!stop && i != poe->numlower - 1 && >>> + d_is_dir(this) && ovl_redirect_dir(dentry->d_sb)) { >>> + err = ovl_check_redirect(this, &redirect); >>> + if (err) >>> + goto out_put; >>> + >>> + if (redirect && poe != dentry->d_sb->s_root->d_fsdata) { >>> + poe = dentry->d_sb->s_root->d_fsdata; >>> + >> >> Now you are about to continue looping until new value of poe->numlower, >> which is >= then olf value of poe->numlower, but 'stack' was allocated >> according to old value of poe->numlower, so aren't you in danger of >> overflowing it? >> >> Please add a comment to explain the purpose of this loop rewind. > > We are jumping to a stack possibly wider than the current one and need > to find the layer where to continue the downward traversal. I'll add > the comment. > > BTW I don't remember having tested this, so it might possibly be > buggy. Automatic multi-layer testing would really be good. What we > basically need is: > > - create normal (two layer) overlay (with interesting constructs, > whiteout, opaque dir, redirect) > - umount > - create three layer overlay where the two lower layers come from the > previous upper/lower layers > - do more interesting things > > There's one such test in xfstests but it would be good to have more. > > Thanks, > Miklos