| Message ID | 1456855928-29913-4-git-send-email-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > NFS translates the inode from the dentry and uses sb from the dentry > parameters. However, using NFS in conjunction with overlayfs, the inodes > associated with dentries may be associated with overlayfs as opposed > to NFS. So, store inode in nfs_open_context and use d_select_inode() > to translate dentry to inode. I don't see how this helps. The dentry and dentry->d_sb that are associated with the open context need to be NFS namespace objects, otherwise all sorts of things, ranging from inode lookup to NFSv4 state recovery are going to break. Trond -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/01/2016 02:46 PM, Trond Myklebust wrote: > On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >> >> From: Goldwyn Rodrigues <rgoldwyn@suse.com> >> >> NFS translates the inode from the dentry and uses sb from the dentry >> parameters. However, using NFS in conjunction with overlayfs, the inodes >> associated with dentries may be associated with overlayfs as opposed >> to NFS. So, store inode in nfs_open_context and use d_select_inode() >> to translate dentry to inode. > > I don't see how this helps. The dentry and dentry->d_sb that are > associated with the open context need to be NFS namespace objects, > otherwise all sorts of things, ranging from inode lookup to NFSv4 > state recovery are going to break. > dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch. Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry?
On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > > > > On 03/01/2016 02:46 PM, Trond Myklebust wrote: >> >> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >>> >>> >>> From: Goldwyn Rodrigues <rgoldwyn@suse.com> >>> >>> NFS translates the inode from the dentry and uses sb from the dentry >>> parameters. However, using NFS in conjunction with overlayfs, the inodes >>> associated with dentries may be associated with overlayfs as opposed >>> to NFS. So, store inode in nfs_open_context and use d_select_inode() >>> to translate dentry to inode. >> >> >> I don't see how this helps. The dentry and dentry->d_sb that are >> associated with the open context need to be NFS namespace objects, >> otherwise all sorts of things, ranging from inode lookup to NFSv4 >> state recovery are going to break. >> > > dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch. > > Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry? No. It is designed the way it is precisely because it needs namespace information. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 2, 2016 at 9:31 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >> >> >> >> On 03/01/2016 02:46 PM, Trond Myklebust wrote: >>> >>> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >>>> >>>> >>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com> >>>> >>>> NFS translates the inode from the dentry and uses sb from the dentry >>>> parameters. However, using NFS in conjunction with overlayfs, the inodes >>>> associated with dentries may be associated with overlayfs as opposed >>>> to NFS. So, store inode in nfs_open_context and use d_select_inode() >>>> to translate dentry to inode. >>> >>> >>> I don't see how this helps. The dentry and dentry->d_sb that are >>> associated with the open context need to be NFS namespace objects, >>> otherwise all sorts of things, ranging from inode lookup to NFSv4 >>> state recovery are going to break. >>> >> >> dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch. >> >> Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry? > > No. It is designed the way it is precisely because it needs namespace > information. IOW: this has never been intended to be an overlayfs object. It needs to reflect the _real_ NFS namespace for various reasons (including recovery). -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 2, 2016 at 3:38 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Wed, Mar 2, 2016 at 9:31 AM, Trond Myklebust > <trond.myklebust@primarydata.com> wrote: >> On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >>> >>> >>> >>> On 03/01/2016 02:46 PM, Trond Myklebust wrote: >>>> >>>> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: >>>>> >>>>> >>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com> >>>>> >>>>> NFS translates the inode from the dentry and uses sb from the dentry >>>>> parameters. However, using NFS in conjunction with overlayfs, the inodes >>>>> associated with dentries may be associated with overlayfs as opposed >>>>> to NFS. So, store inode in nfs_open_context and use d_select_inode() >>>>> to translate dentry to inode. >>>> >>>> >>>> I don't see how this helps. The dentry and dentry->d_sb that are >>>> associated with the open context need to be NFS namespace objects, >>>> otherwise all sorts of things, ranging from inode lookup to NFSv4 >>>> state recovery are going to break. >>>> >>> >>> dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch. >>> >>> Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry? >> >> No. It is designed the way it is precisely because it needs namespace >> information. > > IOW: this has never been intended to be an overlayfs object. It needs > to reflect the _real_ NFS namespace for various reasons (including > recovery). Not sure how any dentry seen by NFS became associated with overlayfs. Through ->f_path by any chance? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/02/2016 08:43 AM, Miklos Szeredi wrote: <snip> > > > Not sure how any dentry seen by NFS became associated with overlayfs. > Through ->f_path by any chance? > Yes, NFS extracts dentry from filp->f_path.dentry in fs/nfs/inode.c nfs_open(). What can it use to evaluate dentry?
On Wed, Mar 2, 2016 at 4:57 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote: > On 03/02/2016 08:43 AM, Miklos Szeredi wrote: >> Not sure how any dentry seen by NFS became associated with overlayfs. >> Through ->f_path by any chance? >> > > Yes, NFS extracts dentry from filp->f_path.dentry in fs/nfs/inode.c > nfs_open(). What can it use to evaluate dentry? Commit 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay") broke this. Breakage only affects regular files. Accessing file->f_path.dentry->d_name (and "%pD" format etc) is OK for everything. ->d_fsdata is not OK. I have no idea what the plan was with filesystems that use f_path.dentry a separate open file for them. David? Al? My plan was to introduce a file_dentry() helper that MUST be used by filesystems to get the dentry from the file and that makes sure it's the right one (check against file_inode()). If not, then we could call into overlayfs to return the right one, similar to ->d_select_inode(), except we want to have a dentry and we want to have *a particular dentry* matching file_inode() (the file could have been copied up in the mean time). Thanks, Miklos Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 9cce670..a9e0ffd 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1446,7 +1446,7 @@ static fmode_t flags_to_mode(int flags) static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags) { - return alloc_nfs_open_context(dentry, flags_to_mode(open_flags)); + return alloc_nfs_open_context(dentry, d_select_inode(dentry, open_flags), flags_to_mode(open_flags)); } static int do_open(struct inode *inode, struct file *filp) diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 86faecf..64fce4b 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -728,7 +728,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx) { struct nfs_lock_context *res, *new = NULL; - struct inode *inode = d_inode(ctx->dentry); + struct inode *inode = ctx->inode; spin_lock(&inode->i_lock); res = __nfs_find_lock_context(ctx); @@ -756,7 +756,7 @@ EXPORT_SYMBOL_GPL(nfs_get_lock_context); void nfs_put_lock_context(struct nfs_lock_context *l_ctx) { struct nfs_open_context *ctx = l_ctx->open_context; - struct inode *inode = d_inode(ctx->dentry); + struct inode *inode = ctx->inode; if (!atomic_dec_and_lock(&l_ctx->count, &inode->i_lock)) return; @@ -785,7 +785,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync) return; if (!is_sync) return; - inode = d_inode(ctx->dentry); + inode = ctx->inode; nfsi = NFS_I(inode); if (inode->i_mapping->nrpages == 0) return; @@ -800,7 +800,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync) } EXPORT_SYMBOL_GPL(nfs_close_context); -struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode) +struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode) { struct nfs_open_context *ctx; struct rpc_cred *cred = rpc_lookup_cred(); @@ -812,8 +812,9 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f put_rpccred(cred); return ERR_PTR(-ENOMEM); } - nfs_sb_active(dentry->d_sb); + nfs_sb_active(inode->i_sb); ctx->dentry = dget(dentry); + ctx->inode = inode; ctx->cred = cred; ctx->state = NULL; ctx->mode = f_mode; @@ -837,8 +838,8 @@ EXPORT_SYMBOL_GPL(get_nfs_open_context); static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync) { - struct inode *inode = d_inode(ctx->dentry); - struct super_block *sb = ctx->dentry->d_sb; + struct inode *inode = ctx->inode; + struct super_block *sb = ctx->inode->i_sb; if (!list_empty(&ctx->list)) { if (!atomic_dec_and_lock(&ctx->lock_context.count, &inode->i_lock)) @@ -874,7 +875,7 @@ static void put_nfs_open_context_sync(struct nfs_open_context *ctx) */ void nfs_inode_attach_open_context(struct nfs_open_context *ctx) { - struct inode *inode = d_inode(ctx->dentry); + struct inode *inode = ctx->inode; struct nfs_inode *nfsi = NFS_I(inode); spin_lock(&inode->i_lock); @@ -917,7 +918,7 @@ void nfs_file_clear_open_context(struct file *filp) struct nfs_open_context *ctx = nfs_file_open_context(filp); if (ctx) { - struct inode *inode = d_inode(ctx->dentry); + struct inode *inode = ctx->inode; /* * We fatal error on write before. Try to writeback @@ -940,7 +941,7 @@ int nfs_open(struct inode *inode, struct file *filp) { struct nfs_open_context *ctx; - ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode); + ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode); if (IS_ERR(ctx)) return PTR_ERR(ctx); nfs_file_set_open_context(filp, ctx); diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 57ca1c8..c7be33d 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp) parent = dget_parent(dentry); dir = d_inode(parent); - ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode); + ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode); err = PTR_ERR(ctx); if (IS_ERR(ctx)) goto out; diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 1488159..57cffb9 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3707,7 +3707,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, struct nfs4_state *state; int status = 0; - ctx = alloc_nfs_open_context(dentry, FMODE_READ); + ctx = alloc_nfs_open_context(dentry, d_select_inode(dentry, flags), FMODE_READ); if (IS_ERR(ctx)) return PTR_ERR(ctx); diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 67300f8..1f18164 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -72,6 +72,7 @@ struct nfs4_state; struct nfs_open_context { struct nfs_lock_context lock_context; struct dentry *dentry; + struct inode *inode; struct rpc_cred *cred; struct nfs4_state *state; fmode_t mode; @@ -361,7 +362,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr, extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx); extern void put_nfs_open_context(struct nfs_open_context *ctx); extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode); -extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode); +extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode); extern void nfs_inode_attach_open_context(struct nfs_open_context *ctx); extern void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx); extern void nfs_file_clear_open_context(struct file *flip);