| Message ID | 20180119103952.GA24104@veci.piliscsaba.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >> > How is this for an option? > [...] >> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >> > +{ >> > + return __d_obtain_alias(inode, 1, fsdata); >> > } >> > EXPORT_SYMBOL(d_obtain_alias); > > It would work, but I like this interface better: > > +extern struct dentry * d_alloc_anon(struct super_block *); > +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); > OK. Thanks for the patch! > And full patch: > > diff --git a/fs/dcache.c b/fs/dcache.c > index b5d5ea984ac4..15dc32178813 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -1699,9 +1699,15 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name) > } > EXPORT_SYMBOL(d_alloc); > > +struct dentry *d_alloc_anon(struct super_block *sb) > +{ > + return __d_alloc(sb, NULL); > +} > +EXPORT_SYMBOL(d_alloc_anon); > + > struct dentry *d_alloc_cursor(struct dentry * parent) > { > - struct dentry *dentry = __d_alloc(parent->d_sb, NULL); > + struct dentry *dentry = d_alloc_anon(parent->d_sb); > if (dentry) { > dentry->d_flags |= DCACHE_RCUACCESS | DCACHE_DENTRY_CURSOR; > dentry->d_parent = dget(parent); > @@ -1887,7 +1893,7 @@ struct dentry *d_make_root(struct inode *root_inode) > struct dentry *res = NULL; > > if (root_inode) { > - res = __d_alloc(root_inode->i_sb, NULL); > + res = d_alloc_anon(root_inode->i_sb); > if (res) > d_instantiate(res, root_inode); > else > @@ -1926,33 +1932,18 @@ struct dentry *d_find_any_alias(struct inode *inode) > } > EXPORT_SYMBOL(d_find_any_alias); > > -static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) > +static struct dentry *__d_instantiate_anon(struct dentry *dentry, > + struct inode *inode, > + bool disconnected) > { > - struct dentry *tmp; > struct dentry *res; > - unsigned add_flags; > - > - if (!inode) > - return ERR_PTR(-ESTALE); > - if (IS_ERR(inode)) > - return ERR_CAST(inode); > - > - res = d_find_any_alias(inode); > - if (res) > - goto out_iput; > > - tmp = __d_alloc(inode->i_sb, NULL); > - if (!tmp) { > - res = ERR_PTR(-ENOMEM); > - goto out_iput; > - } > - > - security_d_instantiate(tmp, inode); > + security_d_instantiate(dentry, inode); > spin_lock(&inode->i_lock); > res = __d_find_any_alias(inode); > if (res) { > spin_unlock(&inode->i_lock); > - dput(tmp); > + dput(dentry); > goto out_iput; > } > > @@ -1962,22 +1953,56 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) > if (disconnected) > add_flags |= DCACHE_DISCONNECTED; > > - spin_lock(&tmp->d_lock); > - __d_set_inode_and_type(tmp, inode, add_flags); > - hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry); > - hlist_bl_lock(&tmp->d_sb->s_anon); > - hlist_bl_add_head(&tmp->d_hash, &tmp->d_sb->s_anon); > - hlist_bl_unlock(&tmp->d_sb->s_anon); > - spin_unlock(&tmp->d_lock); > + spin_lock(&dentry->d_lock); > + __d_set_inode_and_type(dentry, inode, add_flags); > + hlist_add_head(&dentry->d_u.d_alias, &inode->i_dentry); > + hlist_bl_lock(&dentry->d_sb->s_anon); > + hlist_bl_add_head(&dentry->d_hash, &dentry->d_sb->s_anon); > + hlist_bl_unlock(&dentry->d_sb->s_anon); > + spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > > - return tmp; > + return dentry; > > out_iput: > iput(inode); > return res; > } > > +struct dentry *d_instantiate_anon(struct dentry *dentry, struct inode *inode) > +{ > + return __d_instantiate_anon(dentry, inode, true); > +} > +EXPORT_SYMBOL(d_instantiate_anon); > + > +static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected) > +{ > + struct dentry *tmp; > + struct dentry *res; > + unsigned add_flags; > + > + if (!inode) > + return ERR_PTR(-ESTALE); > + if (IS_ERR(inode)) > + return ERR_CAST(inode); > + > + res = d_find_any_alias(inode); > + if (res) > + goto out_iput; > + > + tmp = d_alloc_anon(inode->i_sb); > + if (!tmp) { > + res = ERR_PTR(-ENOMEM); > + goto out_iput; > + } > + > + return __d_instantiate_anon(tmp, inode, disconnected); > + > +out_iput: > + iput(inode); > + return res; > +} > + > /** > * d_obtain_alias - find or allocate a DISCONNECTED dentry for a given inode > * @inode: inode to allocate the dentry for > @@ -1998,7 +2023,7 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) > */ > struct dentry *d_obtain_alias(struct inode *inode) > { > - return __d_obtain_alias(inode, 1); > + return __d_obtain_alias(inode, true); > } > EXPORT_SYMBOL(d_obtain_alias); > > @@ -2019,7 +2044,7 @@ EXPORT_SYMBOL(d_obtain_alias); > */ > struct dentry *d_obtain_root(struct inode *inode) > { > - return __d_obtain_alias(inode, 0); > + return __d_obtain_alias(inode, false); > } > EXPORT_SYMBOL(d_obtain_root); > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c > index 25461781c103..7477f28cb99b 100644 > --- a/fs/overlayfs/export.c > +++ b/fs/overlayfs/export.c > @@ -184,28 +184,32 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb, > return ERR_CAST(inode); > } > > - dentry = d_obtain_alias(inode); > - if (IS_ERR(dentry) || dentry->d_fsdata) > - return dentry; > - > - oe = ovl_alloc_entry(!!lower); > - if (!oe) { > - dput(dentry); > - return ERR_PTR(-ENOMEM); > - } > + if (index) > + ovl_set_flag(OVL_INDEX, inode); > > - dentry->d_fsdata = oe; > - if (upper_alias) > - ovl_dentry_set_upper_alias(dentry); > - if (lower) { > - oe->lowerstack->dentry = dget(lower); > - oe->lowerstack->layer = lowerpath->layer; > + dentry = d_find_any_alias(inode); > + if (!dentry) { > + dentry = d_alloc_anon(inode->i_sb); > + if (!dentry) > + goto nomem; > + oe = ovl_alloc_entry(lower ? 1 : 0); > + if (!oe) > + goto nomem; > + if (lower) { > + oe->lowerstack->dentry = dget(lower); > + oe->lowerstack->layer = lowerpath->layer; > + } > + dentry->d_fsdata = oe; > + if (upper_alias) > + ovl_dentry_set_upper_alias(dentry); > } > > - if (index) > - ovl_set_flag(OVL_INDEX, inode); > + return d_instantiate_anon(dentry, inode); > > - return dentry; > +nomem: > + iput(inode); > + dput(dentry); > + return ERR_PTR(-ENOMEM); > } > > /* Get the upper or lower dentry in stach whose on layer @idx */ > diff --git a/include/linux/dcache.h b/include/linux/dcache.h > index 65cd8ab60b7a..82a99d366aec 100644 > --- a/include/linux/dcache.h > +++ b/include/linux/dcache.h > @@ -227,6 +227,7 @@ extern seqlock_t rename_lock; > */ > extern void d_instantiate(struct dentry *, struct inode *); > extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *); > +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); > extern int d_instantiate_no_diralias(struct dentry *, struct inode *); > extern void __d_drop(struct dentry *dentry); > extern void d_drop(struct dentry *dentry); > @@ -235,6 +236,7 @@ extern void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op > > /* allocate/de-allocate */ > extern struct dentry * d_alloc(struct dentry *, const struct qstr *); > +extern struct dentry * d_alloc_anon(struct super_block *); > extern struct dentry * d_alloc_pseudo(struct super_block *, const struct qstr *); > extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *, > wait_queue_head_t *); >
On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >>> > How is this for an option? >> [...] >>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >>> > +{ >>> > + return __d_obtain_alias(inode, 1, fsdata); >>> > } >>> > EXPORT_SYMBOL(d_obtain_alias); >> >> It would work, but I like this interface better: >> >> +extern struct dentry * d_alloc_anon(struct super_block *); >> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); >> > > OK. Thanks for the patch! > Added your dcache patch to the series and reworked my patches to use the new helpers. Tested result is pushed to: https://github.com/amir73il/linux/commits/ovl-nfs-export-v3 Prep patches changes since v2: - Rebased over fix patch "hash all directory inodes for fsnotify" - Rename mount/config option from "verify" to "nfs_export" - Force r/o mount when index dir creation fails - Allow enabling "nfs_export" for non-upper mount - Require "redirect_dir=nofollow" for non-upper mount - Rename dir index entries xattr from ".origin" to ".upper" - Re-factor ovl_{get|set|verify}_origin() helpers - Simplify test for temp index name (starts with #) - Abandon ovl_dentry_is_renamed() test for lower st_ino - Document overhead on mount with full index - Document change of behavior when verifying lower origin - Added patch to make room in ovl_entry struct NFS export changes since v2: - Fix exportfs ops for r/o overlay with no upperdir - Document reason for copy up directory on encode - Take care of racing with rename while connecting dir - Explain the reasons for choosing the 'connected' dir approach - Do not add dentry without ovl_entry to dcache Optimizations TODO: - Copy up on encode only when lower ancestor is below middle layer redirect - Hash inode by fh to avoid origin decode of whiteout fh As far as I know, the series is now functionally correct and all comments so far addressed. The remaining optimizations will be done on top of this series. Thanks, Amir.
On Fri, Jan 19, 2018 at 9:10 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >>>> > How is this for an option? >>> [...] >>>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >>>> > +{ >>>> > + return __d_obtain_alias(inode, 1, fsdata); >>>> > } >>>> > EXPORT_SYMBOL(d_obtain_alias); >>> >>> It would work, but I like this interface better: >>> >>> +extern struct dentry * d_alloc_anon(struct super_block *); >>> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); >>> >> >> OK. Thanks for the patch! >> > > Added your dcache patch to the series and reworked my patches > to use the new helpers. > > Tested result is pushed to: > https://github.com/amir73il/linux/commits/ovl-nfs-export-v3 > > Prep patches changes since v2: > - Rebased over fix patch "hash all directory inodes for fsnotify" > - Rename mount/config option from "verify" to "nfs_export" > - Force r/o mount when index dir creation fails > - Allow enabling "nfs_export" for non-upper mount > - Require "redirect_dir=nofollow" for non-upper mount > - Rename dir index entries xattr from ".origin" to ".upper" > - Re-factor ovl_{get|set|verify}_origin() helpers > - Simplify test for temp index name (starts with #) > - Abandon ovl_dentry_is_renamed() test for lower st_ino > - Document overhead on mount with full index > - Document change of behavior when verifying lower origin > - Added patch to make room in ovl_entry struct > > NFS export changes since v2: > - Fix exportfs ops for r/o overlay with no upperdir > - Document reason for copy up directory on encode > - Take care of racing with rename while connecting dir > - Explain the reasons for choosing the 'connected' dir approach > - Do not add dentry without ovl_entry to dcache > > Optimizations TODO: > - Copy up on encode only when lower ancestor is below middle layer redirect > - Hash inode by fh to avoid origin decode of whiteout fh > > As far as I know, the series is now functionally correct and all comments > so far addressed. The remaining optimizations will be done on top of this > series. Pushed to overlayfs-next with one fix (do not warn about falling back to nfs_export=off if nfs_export is already off). That spurious warning makes me wonder: how much of the option matrix is tested? Thanks, Miklos
On Wed, Jan 24, 2018 at 12:34 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Fri, Jan 19, 2018 at 9:10 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >>>>> > How is this for an option? >>>> [...] >>>>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >>>>> > +{ >>>>> > + return __d_obtain_alias(inode, 1, fsdata); >>>>> > } >>>>> > EXPORT_SYMBOL(d_obtain_alias); >>>> >>>> It would work, but I like this interface better: >>>> >>>> +extern struct dentry * d_alloc_anon(struct super_block *); >>>> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); >>>> >>> >>> OK. Thanks for the patch! >>> >> >> Added your dcache patch to the series and reworked my patches >> to use the new helpers. >> >> Tested result is pushed to: >> https://github.com/amir73il/linux/commits/ovl-nfs-export-v3 >> >> Prep patches changes since v2: >> - Rebased over fix patch "hash all directory inodes for fsnotify" >> - Rename mount/config option from "verify" to "nfs_export" >> - Force r/o mount when index dir creation fails >> - Allow enabling "nfs_export" for non-upper mount >> - Require "redirect_dir=nofollow" for non-upper mount >> - Rename dir index entries xattr from ".origin" to ".upper" >> - Re-factor ovl_{get|set|verify}_origin() helpers >> - Simplify test for temp index name (starts with #) >> - Abandon ovl_dentry_is_renamed() test for lower st_ino >> - Document overhead on mount with full index >> - Document change of behavior when verifying lower origin >> - Added patch to make room in ovl_entry struct >> >> NFS export changes since v2: >> - Fix exportfs ops for r/o overlay with no upperdir >> - Document reason for copy up directory on encode >> - Take care of racing with rename while connecting dir >> - Explain the reasons for choosing the 'connected' dir approach >> - Do not add dentry without ovl_entry to dcache >> >> Optimizations TODO: >> - Copy up on encode only when lower ancestor is below middle layer redirect >> - Hash inode by fh to avoid origin decode of whiteout fh >> >> As far as I know, the series is now functionally correct and all comments >> so far addressed. The remaining optimizations will be done on top of this >> series. > > Pushed to overlayfs-next with one fix (do not warn about falling back > to nfs_export=off if nfs_export is already off). Good fix. That warning was a late addition to V3 after allowing nfs_export without index for on-upper r/o mount. > > That spurious warning makes me wonder: how much of the option matrix is tested? > Good question... (adding Eryu in CC) I know Eryu tested with default kernel configuration, because one of my tests found a bug with NFS export and redirect_dir=off. (I fixed the specific test to require redirect_dir) I have posted xfstests for overlay NFS export support with samefs and non-samefs configuration and for non-samefs, there are two lower layers not on the same fs. I also have posted a test for non-upper overlay with two lower layers on samefs and non-samefs. One of the overlay xfstests explicitly turns off index with index=off, (overlay/036), so we have coverage for falling back to nfs_export=off when NFS export is enabled by default (as it usually is in my setup). There is an overlay xfstest with multiple lower layers and no-upper and that test shows the warning about falling back to nfs_export=off due to redirect_dir=nofollow requirement (with default kernel configuration). I did not specifically test underlying fs with no xattr support and no file handle support, though the latter was already tested for index with squashfs (no uuid) and the nfs_export=off code is piggy backed in the same place as index=off. Thanks, Amir.
On Wed, Jan 24, 2018 at 1:04 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, Jan 24, 2018 at 12:34 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Fri, Jan 19, 2018 at 9:10 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>> On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >>>>>> > How is this for an option? >>>>> [...] >>>>>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >>>>>> > +{ >>>>>> > + return __d_obtain_alias(inode, 1, fsdata); >>>>>> > } >>>>>> > EXPORT_SYMBOL(d_obtain_alias); >>>>> >>>>> It would work, but I like this interface better: >>>>> >>>>> +extern struct dentry * d_alloc_anon(struct super_block *); >>>>> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); >>>>> >>>> >>>> OK. Thanks for the patch! >>>> >>> >>> Added your dcache patch to the series and reworked my patches >>> to use the new helpers. >>> >>> Tested result is pushed to: >>> https://github.com/amir73il/linux/commits/ovl-nfs-export-v3 >>> >>> Prep patches changes since v2: >>> - Rebased over fix patch "hash all directory inodes for fsnotify" >>> - Rename mount/config option from "verify" to "nfs_export" >>> - Force r/o mount when index dir creation fails >>> - Allow enabling "nfs_export" for non-upper mount >>> - Require "redirect_dir=nofollow" for non-upper mount >>> - Rename dir index entries xattr from ".origin" to ".upper" >>> - Re-factor ovl_{get|set|verify}_origin() helpers >>> - Simplify test for temp index name (starts with #) >>> - Abandon ovl_dentry_is_renamed() test for lower st_ino >>> - Document overhead on mount with full index >>> - Document change of behavior when verifying lower origin >>> - Added patch to make room in ovl_entry struct >>> >>> NFS export changes since v2: >>> - Fix exportfs ops for r/o overlay with no upperdir >>> - Document reason for copy up directory on encode >>> - Take care of racing with rename while connecting dir >>> - Explain the reasons for choosing the 'connected' dir approach >>> - Do not add dentry without ovl_entry to dcache >>> >>> Optimizations TODO: >>> - Copy up on encode only when lower ancestor is below middle layer redirect >>> - Hash inode by fh to avoid origin decode of whiteout fh >>> >>> As far as I know, the series is now functionally correct and all comments >>> so far addressed. The remaining optimizations will be done on top of this >>> series. >> >> Pushed to overlayfs-next with one fix (do not warn about falling back >> to nfs_export=off if nfs_export is already off). > > Good fix. > That warning was a late addition to V3 after allowing nfs_export without > index for on-upper r/o mount. > >> >> That spurious warning makes me wonder: how much of the option matrix is tested? >> > > Good question... (adding Eryu in CC) > > I know Eryu tested with default kernel configuration, because one > of my tests found a bug with NFS export and redirect_dir=off. > (I fixed the specific test to require redirect_dir) > > I have posted xfstests for overlay NFS export support with samefs > and non-samefs configuration and for non-samefs, there are two > lower layers not on the same fs. I also have posted a test for non-upper > overlay with two lower layers on samefs and non-samefs. > > One of the overlay xfstests explicitly turns off index with index=off, > (overlay/036), so we have coverage for falling back to nfs_export=off > when NFS export is enabled by default (as it usually is in my setup). > > There is an overlay xfstest with multiple lower layers and no-upper > and that test shows the warning about falling back to nfs_export=off > due to redirect_dir=nofollow requirement (with default kernel configuration). > > I did not specifically test underlying fs with no xattr support and no > file handle support, though the latter was already tested for index > with squashfs (no uuid) and the nfs_export=off code is piggy backed > in the same place as index=off. > Clarification: it may be implied when I wrote that "we have test coverage..." that there are automatic tests to verify falling back to nfs_export=off. This is not the case. I was only listing "falling back" cases whose warning I regularly see in my routine overlay/quick xfstest as expected. Thanks, Amir.
On Wed, Jan 24, 2018 at 1:18 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, Jan 24, 2018 at 1:04 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Wed, Jan 24, 2018 at 12:34 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>> On Fri, Jan 19, 2018 at 9:10 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>> On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote: >>>>> On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >>>>>> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote: >>>>>>> > How is this for an option? >>>>>> [...] >>>>>>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata) >>>>>>> > +{ >>>>>>> > + return __d_obtain_alias(inode, 1, fsdata); >>>>>>> > } >>>>>>> > EXPORT_SYMBOL(d_obtain_alias); >>>>>> >>>>>> It would work, but I like this interface better: >>>>>> >>>>>> +extern struct dentry * d_alloc_anon(struct super_block *); >>>>>> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); >>>>>> >>>>> >>>>> OK. Thanks for the patch! >>>>> >>>> >>>> Added your dcache patch to the series and reworked my patches >>>> to use the new helpers. >>>> >>>> Tested result is pushed to: >>>> https://github.com/amir73il/linux/commits/ovl-nfs-export-v3 >>>> >>>> Prep patches changes since v2: >>>> - Rebased over fix patch "hash all directory inodes for fsnotify" >>>> - Rename mount/config option from "verify" to "nfs_export" >>>> - Force r/o mount when index dir creation fails >>>> - Allow enabling "nfs_export" for non-upper mount >>>> - Require "redirect_dir=nofollow" for non-upper mount >>>> - Rename dir index entries xattr from ".origin" to ".upper" >>>> - Re-factor ovl_{get|set|verify}_origin() helpers >>>> - Simplify test for temp index name (starts with #) >>>> - Abandon ovl_dentry_is_renamed() test for lower st_ino >>>> - Document overhead on mount with full index >>>> - Document change of behavior when verifying lower origin >>>> - Added patch to make room in ovl_entry struct >>>> >>>> NFS export changes since v2: >>>> - Fix exportfs ops for r/o overlay with no upperdir >>>> - Document reason for copy up directory on encode >>>> - Take care of racing with rename while connecting dir >>>> - Explain the reasons for choosing the 'connected' dir approach >>>> - Do not add dentry without ovl_entry to dcache >>>> >>>> Optimizations TODO: >>>> - Copy up on encode only when lower ancestor is below middle layer redirect >>>> - Hash inode by fh to avoid origin decode of whiteout fh >>>> >>>> As far as I know, the series is now functionally correct and all comments >>>> so far addressed. The remaining optimizations will be done on top of this >>>> series. >>> >>> Pushed to overlayfs-next with one fix (do not warn about falling back >>> to nfs_export=off if nfs_export is already off). >> >> Good fix. >> That warning was a late addition to V3 after allowing nfs_export without >> index for on-upper r/o mount. >> >>> >>> That spurious warning makes me wonder: how much of the option matrix is tested? >>> >> >> Good question... (adding Eryu in CC) >> >> I know Eryu tested with default kernel configuration, because one >> of my tests found a bug with NFS export and redirect_dir=off. >> (I fixed the specific test to require redirect_dir) >> >> I have posted xfstests for overlay NFS export support with samefs >> and non-samefs configuration and for non-samefs, there are two >> lower layers not on the same fs. I also have posted a test for non-upper >> overlay with two lower layers on samefs and non-samefs. >> >> One of the overlay xfstests explicitly turns off index with index=off, >> (overlay/036), so we have coverage for falling back to nfs_export=off >> when NFS export is enabled by default (as it usually is in my setup). >> >> There is an overlay xfstest with multiple lower layers and no-upper >> and that test shows the warning about falling back to nfs_export=off >> due to redirect_dir=nofollow requirement (with default kernel configuration). >> >> I did not specifically test underlying fs with no xattr support and no >> file handle support, though the latter was already tested for index >> with squashfs (no uuid) and the nfs_export=off code is piggy backed >> in the same place as index=off. >> Correction. no file handle support is covered by nested overlay test (overlay/029), so I do see those "falling back" warnings as well with v3. (I did not see the warnings in all my tests because I sometimes apply an extra patch to support NFS export on nested overlay). > > Clarification: it may be implied when I wrote that "we have test coverage..." > that there are automatic tests to verify falling back to nfs_export=off. > This is not the case. I was only listing "falling back" cases whose warning > I regularly see in my routine overlay/quick xfstest as expected. > > Thanks, > Amir.
diff --git a/fs/dcache.c b/fs/dcache.c index b5d5ea984ac4..15dc32178813 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1699,9 +1699,15 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name) } EXPORT_SYMBOL(d_alloc); +struct dentry *d_alloc_anon(struct super_block *sb) +{ + return __d_alloc(sb, NULL); +} +EXPORT_SYMBOL(d_alloc_anon); + struct dentry *d_alloc_cursor(struct dentry * parent) { - struct dentry *dentry = __d_alloc(parent->d_sb, NULL); + struct dentry *dentry = d_alloc_anon(parent->d_sb); if (dentry) { dentry->d_flags |= DCACHE_RCUACCESS | DCACHE_DENTRY_CURSOR; dentry->d_parent = dget(parent); @@ -1887,7 +1893,7 @@ struct dentry *d_make_root(struct inode *root_inode) struct dentry *res = NULL; if (root_inode) { - res = __d_alloc(root_inode->i_sb, NULL); + res = d_alloc_anon(root_inode->i_sb); if (res) d_instantiate(res, root_inode); else @@ -1926,33 +1932,18 @@ struct dentry *d_find_any_alias(struct inode *inode) } EXPORT_SYMBOL(d_find_any_alias); -static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) +static struct dentry *__d_instantiate_anon(struct dentry *dentry, + struct inode *inode, + bool disconnected) { - struct dentry *tmp; struct dentry *res; - unsigned add_flags; - - if (!inode) - return ERR_PTR(-ESTALE); - if (IS_ERR(inode)) - return ERR_CAST(inode); - - res = d_find_any_alias(inode); - if (res) - goto out_iput; - tmp = __d_alloc(inode->i_sb, NULL); - if (!tmp) { - res = ERR_PTR(-ENOMEM); - goto out_iput; - } - - security_d_instantiate(tmp, inode); + security_d_instantiate(dentry, inode); spin_lock(&inode->i_lock); res = __d_find_any_alias(inode); if (res) { spin_unlock(&inode->i_lock); - dput(tmp); + dput(dentry); goto out_iput; } @@ -1962,22 +1953,56 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) if (disconnected) add_flags |= DCACHE_DISCONNECTED; - spin_lock(&tmp->d_lock); - __d_set_inode_and_type(tmp, inode, add_flags); - hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry); - hlist_bl_lock(&tmp->d_sb->s_anon); - hlist_bl_add_head(&tmp->d_hash, &tmp->d_sb->s_anon); - hlist_bl_unlock(&tmp->d_sb->s_anon); - spin_unlock(&tmp->d_lock); + spin_lock(&dentry->d_lock); + __d_set_inode_and_type(dentry, inode, add_flags); + hlist_add_head(&dentry->d_u.d_alias, &inode->i_dentry); + hlist_bl_lock(&dentry->d_sb->s_anon); + hlist_bl_add_head(&dentry->d_hash, &dentry->d_sb->s_anon); + hlist_bl_unlock(&dentry->d_sb->s_anon); + spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); - return tmp; + return dentry; out_iput: iput(inode); return res; } +struct dentry *d_instantiate_anon(struct dentry *dentry, struct inode *inode) +{ + return __d_instantiate_anon(dentry, inode, true); +} +EXPORT_SYMBOL(d_instantiate_anon); + +static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected) +{ + struct dentry *tmp; + struct dentry *res; + unsigned add_flags; + + if (!inode) + return ERR_PTR(-ESTALE); + if (IS_ERR(inode)) + return ERR_CAST(inode); + + res = d_find_any_alias(inode); + if (res) + goto out_iput; + + tmp = d_alloc_anon(inode->i_sb); + if (!tmp) { + res = ERR_PTR(-ENOMEM); + goto out_iput; + } + + return __d_instantiate_anon(tmp, inode, disconnected); + +out_iput: + iput(inode); + return res; +} + /** * d_obtain_alias - find or allocate a DISCONNECTED dentry for a given inode * @inode: inode to allocate the dentry for @@ -1998,7 +2023,7 @@ static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected) */ struct dentry *d_obtain_alias(struct inode *inode) { - return __d_obtain_alias(inode, 1); + return __d_obtain_alias(inode, true); } EXPORT_SYMBOL(d_obtain_alias); @@ -2019,7 +2044,7 @@ EXPORT_SYMBOL(d_obtain_alias); */ struct dentry *d_obtain_root(struct inode *inode) { - return __d_obtain_alias(inode, 0); + return __d_obtain_alias(inode, false); } EXPORT_SYMBOL(d_obtain_root); diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c index 25461781c103..7477f28cb99b 100644 --- a/fs/overlayfs/export.c +++ b/fs/overlayfs/export.c @@ -184,28 +184,32 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb, return ERR_CAST(inode); } - dentry = d_obtain_alias(inode); - if (IS_ERR(dentry) || dentry->d_fsdata) - return dentry; - - oe = ovl_alloc_entry(!!lower); - if (!oe) { - dput(dentry); - return ERR_PTR(-ENOMEM); - } + if (index) + ovl_set_flag(OVL_INDEX, inode); - dentry->d_fsdata = oe; - if (upper_alias) - ovl_dentry_set_upper_alias(dentry); - if (lower) { - oe->lowerstack->dentry = dget(lower); - oe->lowerstack->layer = lowerpath->layer; + dentry = d_find_any_alias(inode); + if (!dentry) { + dentry = d_alloc_anon(inode->i_sb); + if (!dentry) + goto nomem; + oe = ovl_alloc_entry(lower ? 1 : 0); + if (!oe) + goto nomem; + if (lower) { + oe->lowerstack->dentry = dget(lower); + oe->lowerstack->layer = lowerpath->layer; + } + dentry->d_fsdata = oe; + if (upper_alias) + ovl_dentry_set_upper_alias(dentry); } - if (index) - ovl_set_flag(OVL_INDEX, inode); + return d_instantiate_anon(dentry, inode); - return dentry; +nomem: + iput(inode); + dput(dentry); + return ERR_PTR(-ENOMEM); } /* Get the upper or lower dentry in stach whose on layer @idx */ diff --git a/include/linux/dcache.h b/include/linux/dcache.h index 65cd8ab60b7a..82a99d366aec 100644 --- a/include/linux/dcache.h +++ b/include/linux/dcache.h @@ -227,6 +227,7 @@ extern seqlock_t rename_lock; */ extern void d_instantiate(struct dentry *, struct inode *); extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *); +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *); extern int d_instantiate_no_diralias(struct dentry *, struct inode *); extern void __d_drop(struct dentry *dentry); extern void d_drop(struct dentry *dentry); @@ -235,6 +236,7 @@ extern void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op /* allocate/de-allocate */ extern struct dentry * d_alloc(struct dentry *, const struct qstr *); +extern struct dentry * d_alloc_anon(struct super_block *); extern struct dentry * d_alloc_pseudo(struct super_block *, const struct qstr *); extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *, wait_queue_head_t *);