| Message ID | 201312130051.AA00031@FM-323941448.jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh wrote: > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry) > static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, > unsigned int flags) > { > - struct dentry *ret; > + struct inode *inode; > > - ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry); > - return ret; > + inode = btrfs_lookup_dentry(dir, dentry); > + if (IS_ERR(inode)) { > + if (PTR_ERR(inode) == -ENOENT) > + inode = NULL; > + else > + return ERR_CAST(inode); > + } > + > + return d_splice_alias(inode, dentry); btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...) and the expanded back and forth with the DCACHE_NEED_LOOKUP flag. a66e7cc626f42de6c745963fe0d807518fa49d39 added 39e3c9553f34381a1b664c27b0c696a266a5735e removed d_splice_alias has been made robust in a9049376ee05bf966bfe2b081b5071326856890a "make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)" you can drop the error handling from btrfs_lookup completely. The rest looks ok. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, David, On 2013/12/17 0:27, David Sterba wrote: > On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh wrote: >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry) >> static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, >> unsigned int flags) >> { >> - struct dentry *ret; >> + struct inode *inode; >> >> - ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry); >> - return ret; >> + inode = btrfs_lookup_dentry(dir, dentry); >> + if (IS_ERR(inode)) { >> + if (PTR_ERR(inode) == -ENOENT) >> + inode = NULL; >> + else >> + return ERR_CAST(inode); >> + } >> + >> + return d_splice_alias(inode, dentry); > > btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...) > and the expanded back and forth with the DCACHE_NEED_LOOKUP flag. > > a66e7cc626f42de6c745963fe0d807518fa49d39 added > 39e3c9553f34381a1b664c27b0c696a266a5735e removed > > d_splice_alias has been made robust in > a9049376ee05bf966bfe2b081b5071326856890a > "make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)" > > you can drop the error handling from btrfs_lookup completely. d_splice_alias() is called by the following formats if btrfs_lookup_dentry() returns NULL before my patch is applied. d_splice_alias(NULL, dentry); However, d_splice_alias() is called by the following formats when becoming the same situation after my patch is applied. d_splice_alias(-ENOENT, dentry); Therefore, I added the following error handling code. + if (IS_ERR(inode)) { + if (PTR_ERR(inode) == -ENOENT) + inode = NULL; + else + return ERR_CAST(inode); + } Thanks, Tsutomu > > The rest looks ok. > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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/btrfs/inode.c b/fs/btrfs/inode.c index 3b4ffaf..f9b45a7 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4910,7 +4910,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry) return ERR_PTR(ret); if (location.objectid == 0) - return NULL; + return ERR_PTR(-ENOENT); if (location.type == BTRFS_INODE_ITEM_KEY) { inode = btrfs_iget(dir->i_sb, &location, root, NULL); @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry) static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { - struct dentry *ret; + struct inode *inode; - ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry); - return ret; + inode = btrfs_lookup_dentry(dir, dentry); + if (IS_ERR(inode)) { + if (PTR_ERR(inode) == -ENOENT) + inode = NULL; + else + return ERR_CAST(inode); + } + + return d_splice_alias(inode, dentry); } unsigned char btrfs_filetype_table[] = { diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 9d46f60..716779c 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -389,6 +389,7 @@ static noinline int create_subvol(struct inode *dir, struct btrfs_root *new_root; struct btrfs_block_rsv block_rsv; struct timespec cur_time = CURRENT_TIME; + struct inode *inode; int ret; int err; u64 objectid; @@ -550,8 +551,14 @@ fail: if (err && !ret) ret = err; - if (!ret) - d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry)); + if (!ret) { + inode = btrfs_lookup_dentry(dir, dentry); + if (IS_ERR(inode)) { + ret = PTR_ERR(inode); + goto out; + } + d_instantiate(dentry, inode); + } out: btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved); return ret; @@ -639,7 +646,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, ret = PTR_ERR(inode); goto fail; } - BUG_ON(!inode); + d_instantiate(dentry, inode); ret = 0; fail:
Clean up btrfs_lookup_dentry() to never return NULL, but PTR_ERR(-ENOENT) instead. This keeps the return value convention consistent. Callers who use btrfs_lookup_dentry() require a trivial update. create_snapshot() in particular looks like it can also lose a BUG_ON(!inode) which is not really needed - there seems less harm in returning ENOENT to userspace at that point in the stack than there is to crash the machine. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- fs/btrfs/inode.c | 15 +++++++++++---- fs/btrfs/ioctl.c | 13 ++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html