login
Search
The following is a series of patches related to Unionfs. Aside from a few
minor cleanups/fixes, the two main changes are (1) lower nameidata support
so we can stack on nfsv4, and (2) un/likely optimizations. These patches
were tested (where appropriate) on our 2.6.23-rc8 latest code, as well as
the backports to 2.6.{22,21,20,19,18,9} on ext2/3/4, xfs, reiserfs,
nfs2/3/4, jffs2, ramfs, tmpfs, cramfs, and squashfs (where available). See
http://unionfs.filesystems.org/ to download backported unionfs code.
Please pull from the 'master' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/ezk/unionfs.git
to receive the following:
Erez Zadok (22):
Unionfs: display informational messages only if debug is on
Unionfs: cast page->index loff_t before shifting
Unionfs: minor coding style updates
Unionfs: add lower nameidata debugging support
Unionfs: lower nameidata support for nfsv4
Unionfs: add un/likely conditionals on common fileops
Unionfs: add un/likely conditionals on copyup ops
Unionfs: add un/likely conditionals on debug ops
Unionfs: add un/likely conditionals on dentry ops
Unionfs: add un/likely conditionals on dir ops
Unionfs: add un/likely conditionals on headers
Unionfs: add un/likely conditionals on fileops
Unionfs: add un/likely conditionals on inode ops
Unionfs: add un/likely conditionals on lookup ops
Unionfs: add un/likely conditionals on super ops
Unionfs: add un/likely conditionals on mmap ops
Unionfs: add un/likely conditionals on rename ops
Unionfs: add un/likely conditionals on readdir ops
Unionfs: add un/likely conditionals on common subr
Unionfs: add un/likely conditionals on unlink ops
Unionfs: add un/likely conditionals on xattr ops
Unionfs: use poison.h for safe poison pointers
Josef 'Jeff' Sipek (2):
Unionfs: Simplify unionfs_get_nlinks
Unionfs: Remove unused #defines
Olivier Blin (1):
...Fixes bugs in number promotion/demotion computation, as per <http://lkml.org/lkml/2007/9/20/17> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu> Acked-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu> --- fs/unionfs/mmap.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c index 88ef6a6..37af979 100644 --- a/fs/unionfs/mmap.c +++ b/fs/unionfs/mmap.c @@ -179,7 +179,8 @@ static int unionfs_do_readpage(struct file *file, struct page *page) * may be a little slower, but a lot safer, as the VFS does a lot of * the necessary magic for us. */ - offset = lower_file->f_pos = (page->index << PAGE_CACHE_SHIFT); + offset = lower_file->f_pos = + ((loff_t) page->index << PAGE_CACHE_SHIFT); old_fs = get_fs(); set_fs(KERNEL_DS); err = vfs_read(lower_file, page_data, PAGE_CACHE_SIZE, @@ -289,7 +290,7 @@ static int unionfs_commit_write(struct file *file, struct page *page, BUG_ON(lower_file == NULL); page_data = (char *)kmap(page); - lower_file->f_pos = (page->index << PAGE_CACHE_SHIFT) + from; + lower_file->f_pos = ((loff_t) page->index << PAGE_CACHE_SHIFT) + from; /* * SP: I use vfs_write instead of copying page data and the -- 1.5.2.2 -
It's better to use te page_offset helper as that avoids any confusion on where to cast. -
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/xattr.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/unionfs/xattr.c b/fs/unionfs/xattr.c
index 7f77d7d..bd2de06 100644
--- a/fs/unionfs/xattr.c
+++ b/fs/unionfs/xattr.c
@@ -23,14 +23,14 @@ void *unionfs_xattr_alloc(size_t size, size_t limit)
{
void *ptr;
- if (size > limit)
+ if (unlikely(size > limit))
return ERR_PTR(-E2BIG);
if (!size) /* size request, no buffer is needed */
return NULL;
ptr = kmalloc(size, GFP_KERNEL);
- if (!ptr)
+ if (unlikely(!ptr))
return ERR_PTR(-ENOMEM);
return ptr;
}
@@ -48,7 +48,7 @@ ssize_t unionfs_getxattr(struct dentry *dentry, const char *name, void *value,
unionfs_read_lock(dentry->d_sb);
unionfs_lock_dentry(dentry);
- if (!__unionfs_d_revalidate_chain(dentry, NULL, false)) {
+ if (unlikely(!__unionfs_d_revalidate_chain(dentry, NULL, false))) {
err = -ESTALE;
goto out;
}
@@ -77,7 +77,7 @@ int unionfs_setxattr(struct dentry *dentry, const char *name,
unionfs_read_lock(dentry->d_sb);
unionfs_lock_dentry(dentry);
- if (!__unionfs_d_revalidate_chain(dentry, NULL, false)) {
+ if (unlikely(!__unionfs_d_revalidate_chain(dentry, NULL, false))) {
err = -ESTALE;
goto out;
}
@@ -106,7 +106,7 @@ int unionfs_removexattr(struct dentry *dentry, const char *name)
unionfs_read_lock(dentry->d_sb);
unionfs_lock_dentry(dentry);
- if (!__unionfs_d_revalidate_chain(dentry, NULL, false)) {
+ if (unlikely(!__unionfs_d_revalidate_chain(dentry, NULL, false))) {
err = -ESTALE;
goto out;
}
@@ -135,7 +135,7 @@ ssize_t unionfs_listxattr(struct dentry *dentry, char *list, size_t size)
unionfs_read_lock(dentry->d_sb);
unionfs_lock_dentry(dentry);
- if (!__unionfs_d_revalidate_chain(dentry, NULL, false)) {
+ if (unlikely(!__unionfs_d_revalidate_chain(dentry, NULL, false))) {
err = -ESTALE;
goto out;
}
--
1.5.2.2
-
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/fanout.h | 13 ++++++++-----
fs/unionfs/union.h | 4 ++--
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/fs/unionfs/fanout.h b/fs/unionfs/fanout.h
index 51aa0de..6405399 100644
--- a/fs/unionfs/fanout.h
+++ b/fs/unionfs/fanout.h
@@ -308,17 +308,20 @@ static inline void unionfs_copy_attr_times(struct inode *upper)
int bindex;
struct inode *lower;
- if (!upper || ibstart(upper) < 0)
+ if (unlikely(!upper || ibstart(upper) < 0))
return;
for (bindex=ibstart(upper); bindex <= ibend(upper); bindex++) {
lower = unionfs_lower_inode_idx(upper, bindex);
- if (!lower)
+ if (unlikely(!lower))
continue; /* not all lower dir objects may exist */
- if (timespec_compare(&upper->i_mtime, &lower->i_mtime) < 0)
+ if (unlikely(timespec_compare(&upper->i_mtime,
+ &lower->i_mtime) < 0))
upper->i_mtime = lower->i_mtime;
- if (timespec_compare(&upper->i_ctime, &lower->i_ctime) < 0)
+ if (likely(timespec_compare(&upper->i_ctime,
+ &lower->i_ctime) < 0))
upper->i_ctime = lower->i_ctime;
- if (timespec_compare(&upper->i_atime, &lower->i_atime) < 0)
+ if (likely(timespec_compare(&upper->i_atime,
+ &lower->i_atime) < 0))
upper->i_atime = lower->i_atime;
}
}
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index d27844d..8df44a9 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -472,7 +472,7 @@ static inline struct vfsmount *unionfs_mntget(struct dentry *dentry,
mnt = mntget(unionfs_lower_mnt_idx(dentry, bindex));
#ifdef CONFIG_UNION_FS_DEBUG
- if (!mnt)
+ if (unlikely(!mnt))
printk(KERN_DEBUG "unionfs_mntget: mnt=%p bindex=%d\n",
mnt, bindex);
#endif /* CONFIG_UNION_FS_DEBUG */
@@ -484,7 +484,7 @@ static inline void unionfs_mntput(struct dentry *dentry, int bindex)
{
struct vfsmount *mnt;
- if (!dentry && bindex < 0)
+ if (unlikely(!dentry && bindex < 0))
return;
BUG_ON(!dentry || bindex < ...This is to avoid filling the console/logs with messages that are primarily
of debugging use.
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
Acked-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
---
fs/unionfs/commonfops.c | 4 ++--
fs/unionfs/dentry.c | 6 +++---
fs/unionfs/union.h | 4 ++++
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 87cbb09..e69ccf6 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -394,8 +394,8 @@ int unionfs_file_revalidate(struct file *file, bool willwrite)
if (willwrite && IS_WRITE_FLAG(file->f_flags) &&
!IS_WRITE_FLAG(unionfs_lower_file(file)->f_flags) &&
is_robranch(dentry)) {
- printk(KERN_DEBUG "unionfs: do delay copyup of \"%s\"\n",
- dentry->d_name.name);
+ dprintk(KERN_DEBUG "unionfs: do delay copyup of \"%s\"\n",
+ dentry->d_name.name);
err = do_delayed_copyup(file);
}
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 9e0742d..08b5722 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -46,9 +46,9 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
/* if the dentry is unhashed, do NOT revalidate */
if (d_deleted(dentry)) {
- printk(KERN_DEBUG "unionfs: unhashed dentry being "
- "revalidated: %*s\n",
- dentry->d_name.len, dentry->d_name.name);
+ dprintk(KERN_DEBUG "unionfs: unhashed dentry being "
+ "revalidated: %*s\n",
+ dentry->d_name.len, dentry->d_name.name);
goto out;
}
diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h
index 140b8ae..5e9843b 100644
--- a/fs/unionfs/union.h
+++ b/fs/unionfs/union.h
@@ -507,6 +507,8 @@ static inline void unionfs_mntput(struct dentry *dentry, int bindex)
#ifdef CONFIG_UNION_FS_DEBUG
+#define dprintk(args...) printk(args)
+
/* useful for tracking code reachability */
#define UDBG printk("DBG:%s:%s:%d\n",__FILE__,__FUNCTION__,__LINE__)
@@ -543,6 +545,8 @@ extern void ...Don't we have pr_debug() for that? -
Jan, what's the policy on people writing their own debugging printk systems. I've looked at what other file systems do, and found out that it varies a lot: some use a simple mapping wrapper from printk->dprintk, while others define complex ways to have debugging levels/bitmaps where users can selectively decide (say, by a mount option), what debugging output they'd like to see or not. Here's a small sampling of what some file systems use: 9p/fid.c: P9_DPRINTK(P9_DEBUG_VFS, "fid %d dentry %s\n", autofs/inode.c: DPRINTK(("autofs: shutting down\n")); lockd/svcproc.c: dprintk("lockd: GRANTED_RES called\n"); ncpfs/dir.c: DDPRINTK("ncp_lookup_validate: result=%d\n", val); nfs/client.c: dprintk("<-- nfs_free_client()\n"); nfsd/export.c: dprintk("found domain %s\n", buf); partitions/efi.c: Dprintk("GPT my_lba incorrect: %lld != %lld\n", Not much standardization there :-) but "dprintk" does appear to be the more common use. I wanted something simple to allow me to not printk something that's just for informational/debugging purposes, but w/o having to #ifdef the printk in question, or define a complex debugging-level printk system. Now, looking at pr_debug (in linux/kernel.h), and indeed some filesystems (e.g., affs and configfs) use it. But pr_debug is only active when #define DEBUG is on (not CONFIG_DEBUG). I didn't see a config option that enable DEBUG: is there one? From other file systems, it appears that the more common convention is for filesystem XYZ to offer a config option CONFIG_DEBUG_XYZ, which makes its debugging more self-contained and keeps the config option closer to where the f/s itself is enabled. Thanks, Erez. -
Oh that's either old code or code that has not been reviewed properly. Fact is that I have been made aware of pr_debug() "often enough" Surprise, pr_debug() is just that all nicely wrapped up. Want debug? Do it like this. #ifdef CONFIG_UNIONFS_DEBUG # define DEBUG 1 #endif No I do not think so. But when you grep for dprintk, you should also grep for DEBUG, to be fair :p -
In message <Pine.LNX.4.64.0709261720330.7066@fbirervta.pbzchgretzou.qr>, Jan Engelhardt writes: [...] Sounds good. I'll do that. Thanks, Erez. -
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/rdstate.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/fs/unionfs/rdstate.c b/fs/unionfs/rdstate.c
index 0a18d5c..7ec7f95 100644
--- a/fs/unionfs/rdstate.c
+++ b/fs/unionfs/rdstate.c
@@ -45,7 +45,7 @@ int unionfs_init_filldir_cache(void)
void unionfs_destroy_filldir_cache(void)
{
- if (unionfs_filldir_cachep)
+ if (likely(unionfs_filldir_cachep))
kmem_cache_destroy(unionfs_filldir_cachep);
}
@@ -72,7 +72,8 @@ static int guesstimate_hash_size(struct inode *inode)
return UNIONFS_I(inode)->hashsize;
for (bindex = ibstart(inode); bindex <= ibend(inode); bindex++) {
- if (!(lower_inode = unionfs_lower_inode_idx(inode, bindex)))
+ lower_inode = unionfs_lower_inode_idx(inode, bindex);
+ if (unlikely(!lower_inode))
continue;
if (lower_inode->i_size == DENTPAGE)
@@ -136,7 +137,7 @@ struct unionfs_dir_state *alloc_rdstate(struct inode *inode, int bindex)
sizeof(struct list_head);
rdstate = kmalloc(mallocsize, GFP_KERNEL);
- if (!rdstate)
+ if (unlikely(!rdstate))
return NULL;
spin_lock(&UNIONFS_I(inode)->rdlock);
@@ -217,7 +218,7 @@ struct filldir_node *find_filldir_node(struct unionfs_dir_state *rdstate,
* if the duplicate is in this branch, then the file
* system is corrupted.
*/
- if (cursor->bindex == rdstate->bindex) {
+ if (unlikely(cursor->bindex == rdstate->bindex)) {
printk(KERN_DEBUG "unionfs: filldir: possible "
"I/O error: a file is duplicated "
"in the same branch %d: %s\n",
@@ -227,7 +228,7 @@ struct filldir_node *find_filldir_node(struct unionfs_dir_state *rdstate,
}
}
- if (!found)
+ if (unlikely(!found))
cursor = NULL;
return cursor;
@@ -249,7 +250,7 @@ int add_filldir_node(struct unionfs_dir_state *rdstate, const char *name,
head = &(rdstate->list[index]);
new = kmem_cache_alloc(unionfs_filldir_cachep, GFP_KERNEL);
- if (!new) ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/mmap.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/fs/unionfs/mmap.c b/fs/unionfs/mmap.c
index 37af979..1cea075 100644
--- a/fs/unionfs/mmap.c
+++ b/fs/unionfs/mmap.c
@@ -84,7 +84,7 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
* resort to RAIF's page pointer flipping trick.)
*/
lower_page = find_lock_page(lower_inode->i_mapping, page->index);
- if (!lower_page) {
+ if (unlikely(!lower_page)) {
err = AOP_WRITEPAGE_ACTIVATE;
set_page_dirty(page);
goto out;
@@ -102,7 +102,7 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
BUG_ON(!lower_inode->i_mapping->a_ops->writepage);
/* workaround for some lower file systems: see big comment on top */
- if (wbc->for_writepages && !wbc->fs_private)
+ if (unlikely(wbc->for_writepages && !wbc->fs_private))
wbc->for_writepages = 0;
/* call lower writepage (expects locked page) */
@@ -111,12 +111,12 @@ static int unionfs_writepage(struct page *page, struct writeback_control *wbc)
wbc->for_writepages = saved_for_writepages; /* restore value */
/* b/c find_lock_page locked it and ->writepage unlocks on success */
- if (err)
+ if (unlikely(err))
unlock_page(lower_page);
/* b/c grab_cache_page increased refcnt */
page_cache_release(lower_page);
- if (err < 0) {
+ if (unlikely(err < 0)) {
ClearPageUptodate(page);
goto out;
}
@@ -160,7 +160,7 @@ static int unionfs_do_readpage(struct file *file, struct page *page)
char *page_data = NULL;
loff_t offset;
- if (!UNIONFS_F(file)) {
+ if (unlikely(!UNIONFS_F(file))) {
err = -ENOENT;
goto out;
}
@@ -189,7 +189,7 @@ static int unionfs_do_readpage(struct file *file, struct page *page)
kunmap(page);
- if (err < 0)
+ if (unlikely(err < 0))
goto out;
err = 0;
@@ -199,7 +199,7 @@ static int unionfs_do_readpage(struct file ...Pass nameidata structures as needed to the lower file system, support LOOKUP_ACCESS/OPEN intents. This makes unionfs work on top of nfsv4. Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu> Acked-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu> --- fs/unionfs/dentry.c | 11 +++++++++-- fs/unionfs/inode.c | 8 +++++++- fs/unionfs/lookup.c | 20 +++++++++++++++++--- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c index b21f1e3..52bcb18 100644 --- a/fs/unionfs/dentry.c +++ b/fs/unionfs/dentry.c @@ -156,8 +156,15 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry, if (!lower_dentry || !lower_dentry->d_op || !lower_dentry->d_op->d_revalidate) continue; - if (!lower_dentry->d_op->d_revalidate(lower_dentry, - &lowernd)) + /* + * Don't pass nameidata to lower file system, because we + * don't want an arbitrary lower file being opened or + * returned to us: it may be useless to us because of the + * fanout nature of unionfs (cf. file/directory open-file + * invariants). We will open lower files as and when needed + * later on. + */ + if (!lower_dentry->d_op->d_revalidate(lower_dentry, NULL)) valid = false; } diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c index f8b2c88..7ee4760 100644 --- a/fs/unionfs/inode.c +++ b/fs/unionfs/inode.c @@ -27,6 +27,7 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry, struct dentry *lower_parent_dentry = NULL; char *name = NULL; int valid = 0; + struct nameidata lower_nd; unionfs_read_lock(dentry->d_sb); unionfs_lock_dentry(dentry); @@ -113,7 +114,12 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry, goto out; } - err = vfs_create(lower_parent_dentry->d_inode, lower_dentry, mode, nd); + err = init_lower_nd(&lower_nd, LOOKUP_CREATE); + if (err < 0) + goto out; + err = vfs_create(lower_parent_dentry->d_inode, lower_dentry, mode, + ...
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/debug.c | 20 ++++++++++++++++++++
fs/unionfs/dentry.c | 4 +++-
fs/unionfs/inode.c | 8 +++++++-
fs/unionfs/union.h | 4 ++++
4 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/fs/unionfs/debug.c b/fs/unionfs/debug.c
index 2d15fb0..9546a41 100644
--- a/fs/unionfs/debug.c
+++ b/fs/unionfs/debug.c
@@ -415,6 +415,26 @@ void __unionfs_check_file(const struct file *file,
__unionfs_check_dentry(dentry,fname,fxn,line);
}
+void __unionfs_check_nd(const struct nameidata *nd,
+ const char *fname, const char *fxn, int line)
+{
+ struct file *file;
+ int printed_caller = 0;
+
+ if (!nd)
+ return;
+ if (nd->flags & LOOKUP_OPEN) {
+ file = nd->intent.open.file;
+ if (file->f_path.dentry &&
+ strcmp(file->f_dentry->d_sb->s_type->name, "unionfs")) {
+ PRINT_CALLER(fname, fxn, line);
+ printk(" CND1: lower_file of type %s\n",
+ file->f_path.dentry->d_sb->s_type->name);
+ BUG();
+ }
+ }
+}
+
/* useful to track vfsmount leaks that could cause EBUSY on unmount */
void __show_branch_counts(const struct super_block *sb,
const char *file, const char *fxn, int line)
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index d9bb199..b21f1e3 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -418,8 +418,10 @@ static int unionfs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
unionfs_lock_dentry(dentry);
err = __unionfs_d_revalidate_chain(dentry, nd, false);
unionfs_unlock_dentry(dentry);
- if (err > 0) /* true==1: dentry is valid */
+ if (err > 0) { /* true==1: dentry is valid */
unionfs_check_dentry(dentry);
+ unionfs_check_nd(nd);
+ }
unionfs_read_unlock(dentry->d_sb);
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index de78e26..f8b2c88 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -138,8 +138,10 @@ out:
unionfs_read_unlock(dentry->d_sb);
unionfs_check_inode(parent);
- if ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/sioq.c | 4 ++--
fs/unionfs/subr.c | 26 +++++++++++++-------------
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/fs/unionfs/sioq.c b/fs/unionfs/sioq.c
index 2a8c88e..35d9fc3 100644
--- a/fs/unionfs/sioq.c
+++ b/fs/unionfs/sioq.c
@@ -28,7 +28,7 @@ int __init init_sioq(void)
int err;
superio_workqueue = create_workqueue("unionfs_siod");
- if (!IS_ERR(superio_workqueue))
+ if (unlikely(!IS_ERR(superio_workqueue)))
return 0;
err = PTR_ERR(superio_workqueue);
@@ -39,7 +39,7 @@ int __init init_sioq(void)
void stop_sioq(void)
{
- if (superio_workqueue)
+ if (likely(superio_workqueue))
destroy_workqueue(superio_workqueue);
}
diff --git a/fs/unionfs/subr.c b/fs/unionfs/subr.c
index 6b93b64..6067d65 100644
--- a/fs/unionfs/subr.c
+++ b/fs/unionfs/subr.c
@@ -40,7 +40,7 @@ int create_whiteout(struct dentry *dentry, int start)
/* create dentry's whiteout equivalent */
name = alloc_whname(dentry->d_name.name, dentry->d_name.len);
- if (IS_ERR(name)) {
+ if (unlikely(IS_ERR(name))) {
err = PTR_ERR(name);
goto out;
}
@@ -60,7 +60,7 @@ int create_whiteout(struct dentry *dentry, int start)
dentry,
dentry->d_name.name,
bindex);
- if (!lower_dentry || IS_ERR(lower_dentry)) {
+ if (unlikely(!lower_dentry || IS_ERR(lower_dentry))) {
printk(KERN_DEBUG "unionfs: create_parents "
"failed for bindex = %d\n", bindex);
continue;
@@ -70,7 +70,7 @@ int create_whiteout(struct dentry *dentry, int start)
lower_wh_dentry =
lookup_one_len(name, lower_dentry->d_parent,
dentry->d_name.len + UNIONFS_WHLEN);
- if (IS_ERR(lower_wh_dentry))
+ if (unlikely(IS_ERR(lower_wh_dentry)))
continue;
/*
@@ -84,7 +84,7 @@ int create_whiteout(struct dentry *dentry, int start)
}
err = init_lower_nd(&nd, LOOKUP_CREATE);
- if (err < 0)
+ if (unlikely(err < 0))
goto out;
...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/debug.c | 6 ++++--
fs/unionfs/dentry.c | 2 +-
fs/unionfs/inode.c | 14 ++++++++------
fs/unionfs/main.c | 4 ++--
fs/unionfs/union.h | 2 +-
5 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/fs/unionfs/debug.c b/fs/unionfs/debug.c
index f678534..2d15fb0 100644
--- a/fs/unionfs/debug.c
+++ b/fs/unionfs/debug.c
@@ -467,7 +467,8 @@ void __show_dinode_times(const struct dentry *dentry,
lower_inode = unionfs_lower_inode_idx(inode, bindex);
if (!lower_inode)
continue;
- printk("DT(%s:%lu:%d): ", dentry->d_name.name, inode->i_ino, bindex);
+ printk("DT(%s:%lu:%d): ", dentry->d_name.name, inode->i_ino,
+ bindex);
printk("%s:%s:%d ",file,fxn,line);
printk("um=%lu/%lu lm=%lu/%lu ",
inode->i_mtime.tv_sec, inode->i_mtime.tv_nsec,
@@ -490,7 +491,8 @@ void __show_inode_counts(const struct inode *inode,
printk("SiC: Null inode\n");
return;
}
- for (bindex=sbstart(inode->i_sb); bindex <= sbend(inode->i_sb); bindex++) {
+ for (bindex=sbstart(inode->i_sb); bindex <= sbend(inode->i_sb);
+ bindex++) {
lower_inode = unionfs_lower_inode_idx(inode, bindex);
if (!lower_inode)
continue;
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 08b5722..d9bb199 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -26,7 +26,7 @@
* Returns true if valid, false otherwise.
*/
static bool __unionfs_d_revalidate_one(struct dentry *dentry,
- struct nameidata *nd)
+ struct nameidata *nd)
{
bool valid = true; /* default is valid */
struct dentry *lower_dentry;
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 9638b64..de78e26 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -99,7 +99,8 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
* if lower_dentry is NULL, create the entire
* dentry directory structure in branch 0.
*/
- lower_dentry = ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/unlink.c | 32 ++++++++++++++++----------------
1 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/unionfs/unlink.c b/fs/unionfs/unlink.c
index 3924f7f..33d08d9 100644
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -26,13 +26,13 @@ static int unionfs_unlink_whiteout(struct inode *dir, struct dentry *dentry)
int bindex;
int err = 0;
- if ((err = unionfs_partial_lookup(dentry)))
+ if (unlikely((err = unionfs_partial_lookup(dentry))))
goto out;
bindex = dbstart(dentry);
lower_dentry = unionfs_lower_dentry_idx(dentry, bindex);
- if (!lower_dentry)
+ if (unlikely(!lower_dentry))
goto out;
lower_dir_dentry = lock_parent(lower_dentry);
@@ -42,13 +42,13 @@ static int unionfs_unlink_whiteout(struct inode *dir, struct dentry *dentry)
if (!(err = is_robranch_super(dentry->d_sb, bindex)))
err = vfs_unlink(lower_dir_dentry->d_inode, lower_dentry);
/* if vfs_unlink succeeded, update our inode's times */
- if (!err)
+ if (likely(!err))
unionfs_copy_attr_times(dentry->d_inode);
dput(lower_dentry);
fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
unlock_dir(lower_dir_dentry);
- if (err && !IS_COPYUP_ERR(err))
+ if (unlikely(err && !IS_COPYUP_ERR(err)))
goto out;
if (err) {
@@ -62,11 +62,11 @@ static int unionfs_unlink_whiteout(struct inode *dir, struct dentry *dentry)
err = create_whiteout(dentry, dbstart(dentry));
out:
- if (!err)
+ if (likely(!err))
dentry->d_inode->i_nlink--;
/* We don't want to leave negative leftover dentries for revalidate. */
- if (!err && (dbopaque(dentry) != -1))
+ if (likely(!err && (dbopaque(dentry) != -1)))
update_bstart(dentry);
return err;
@@ -79,7 +79,7 @@ int unionfs_unlink(struct inode *dir, struct dentry *dentry)
unionfs_read_lock(dentry->d_sb);
unionfs_lock_dentry(dentry);
- if (!__unionfs_d_revalidate_chain(dentry, NULL, false)) {
+ if ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/file.c | 38 +++++++++++++++++++-------------------
1 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/fs/unionfs/file.c b/fs/unionfs/file.c
index d8eaaa5..06ca1fa 100644
--- a/fs/unionfs/file.c
+++ b/fs/unionfs/file.c
@@ -24,13 +24,13 @@ static ssize_t unionfs_read(struct file *file, char __user *buf,
int err;
unionfs_read_lock(file->f_path.dentry->d_sb);
- if ((err = unionfs_file_revalidate(file, false)))
+ if (unlikely((err = unionfs_file_revalidate(file, false))))
goto out;
unionfs_check_file(file);
err = do_sync_read(file, buf, count, ppos);
- if (err >= 0)
+ if (likely(err >= 0))
touch_atime(unionfs_lower_mnt(file->f_path.dentry),
unionfs_lower_dentry(file->f_path.dentry));
@@ -47,16 +47,16 @@ static ssize_t unionfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
struct file *file = iocb->ki_filp;
unionfs_read_lock(file->f_path.dentry->d_sb);
- if ((err = unionfs_file_revalidate(file, false)))
+ if (unlikely((err = unionfs_file_revalidate(file, false))))
goto out;
unionfs_check_file(file);
err = generic_file_aio_read(iocb, iov, nr_segs, pos);
- if (err == -EIOCBQUEUED)
+ if (unlikely(err == -EIOCBQUEUED))
err = wait_on_sync_kiocb(iocb);
- if (err >= 0)
+ if (likely(err >= 0))
touch_atime(unionfs_lower_mnt(file->f_path.dentry),
unionfs_lower_dentry(file->f_path.dentry));
@@ -72,13 +72,13 @@ static ssize_t unionfs_write(struct file *file, const char __user *buf,
int err = 0;
unionfs_read_lock(file->f_path.dentry->d_sb);
- if ((err = unionfs_file_revalidate(file, true)))
+ if (unlikely((err = unionfs_file_revalidate(file, true))))
goto out;
unionfs_check_file(file);
err = do_sync_write(file, buf, count, ppos);
/* update our inode times upon a successful lower write */
- if (err >= 0) {
+ if (likely(err >= 0)) {
unionfs_copy_attr_times(file->f_path.dentry->d_inode);
...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/lookup.c | 44 ++++++++++++++++++++++----------------------
1 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/fs/unionfs/lookup.c b/fs/unionfs/lookup.c
index 2109714..92b5e0a 100644
--- a/fs/unionfs/lookup.c
+++ b/fs/unionfs/lookup.c
@@ -59,7 +59,7 @@ static noinline int is_opaque_dir(struct dentry *dentry, int bindex)
mutex_unlock(&lower_inode->i_mutex);
- if (IS_ERR(wh_lower_dentry)) {
+ if (unlikely(IS_ERR(wh_lower_dentry))) {
err = PTR_ERR(wh_lower_dentry);
goto out;
}
@@ -119,12 +119,12 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
case INTERPOSE_PARTIAL:
break;
case INTERPOSE_LOOKUP:
- if ((err = new_dentry_private_data(dentry)))
+ if (unlikely((err = new_dentry_private_data(dentry))))
goto out;
break;
default:
/* default: can only be INTERPOSE_REVAL/REVAL_NEG */
- if ((err = realloc_dentry_private_data(dentry)))
+ if (unlikely((err = realloc_dentry_private_data(dentry))))
goto out;
break;
}
@@ -147,7 +147,7 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
namelen = dentry->d_name.len;
/* No dentries should get created for possible whiteout names. */
- if (!is_validname(name)) {
+ if (unlikely(!is_validname(name))) {
err = -EPERM;
goto out_free;
}
@@ -179,7 +179,7 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
unionfs_lower_dentry_idx(parent_dentry, bindex);
/* if the parent lower dentry does not exist skip this */
- if (!(lower_dir_dentry && lower_dir_dentry->d_inode))
+ if (unlikely(!(lower_dir_dentry && lower_dir_dentry->d_inode)))
continue;
/* also skip it if the parent isn't a directory. */
@@ -189,7 +189,7 @@ struct dentry *unionfs_lookup_backend(struct dentry *dentry,
/* Reuse the whiteout name because its value doesn't change. */
if (!whname) {
whname = alloc_whname(name, namelen);
- if (IS_ERR(whname)) {
+ if ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/dirfops.c | 22 +++++++++++-----------
fs/unionfs/dirhelper.c | 30 +++++++++++++++---------------
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/fs/unionfs/dirfops.c b/fs/unionfs/dirfops.c
index c923e58..fa2df88 100644
--- a/fs/unionfs/dirfops.c
+++ b/fs/unionfs/dirfops.c
@@ -63,7 +63,7 @@ static int unionfs_filldir(void *dirent, const char *name, int namelen,
off_t pos = rdstate2offset(buf->rdstate);
u64 unionfs_ino = ino;
- if (!err) {
+ if (likely(!err)) {
err = buf->filldir(buf->dirent, name, namelen, pos,
unionfs_ino, d_type);
buf->rdstate->offset++;
@@ -74,7 +74,7 @@ static int unionfs_filldir(void *dirent, const char *name, int namelen,
* If we did fill it, stuff it in our hash, otherwise return an
* error.
*/
- if (err) {
+ if (unlikely(err)) {
buf->filldir_error = err;
goto out;
}
@@ -99,7 +99,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
unionfs_read_lock(file->f_path.dentry->d_sb);
- if ((err = unionfs_file_revalidate(file, false)))
+ if (unlikely((err = unionfs_file_revalidate(file, false))))
goto out;
inode = file->f_path.dentry->d_inode;
@@ -110,7 +110,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
goto out;
} else if (file->f_pos > 0) {
uds = find_rdstate(inode, file->f_pos);
- if (!uds) {
+ if (unlikely(!uds)) {
err = -ESTALE;
goto out;
}
@@ -124,7 +124,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
while (uds->bindex <= bend) {
lower_file = unionfs_lower_file_idx(file, uds->bindex);
- if (!lower_file) {
+ if (unlikely(!lower_file)) {
uds->bindex++;
uds->dirpos = 0;
continue;
@@ -141,7 +141,7 @@ static int unionfs_readdir(struct file *file, void *dirent, filldir_t filldir)
/* Read starting from where we last left off. */
offset ...Is it bad to leave this assignment within the unlikely()? -
I don't know. Anyone? Will un/likely "break" in the above form? Actually, it looks like assignments within conditionals are just prohibited (even if you use double parentheses to shut gcc up :-). It's not mentioned in the CodingStyle, but it is in scripts/checkpatch.pl. So I'll have to take out all those assignments out of the conditionals anyway. Good. It makes code more readable (and besides, most modern compilers will optimize it just the same). Cheers, Erez. -
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/rename.c | 96 +++++++++++++++++++++++++-------------------------
1 files changed, 48 insertions(+), 48 deletions(-)
diff --git a/fs/unionfs/rename.c b/fs/unionfs/rename.c
index 7b8fe39..92c4515 100644
--- a/fs/unionfs/rename.c
+++ b/fs/unionfs/rename.c
@@ -39,7 +39,7 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
create_parents(new_dentry->d_parent->d_inode,
new_dentry, new_dentry->d_name.name,
bindex);
- if (IS_ERR(lower_new_dentry)) {
+ if (unlikely(IS_ERR(lower_new_dentry))) {
printk(KERN_DEBUG "unionfs: error creating directory "
"tree for rename, bindex = %d, err = %ld\n",
bindex, PTR_ERR(lower_new_dentry));
@@ -50,7 +50,7 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
wh_name = alloc_whname(new_dentry->d_name.name,
new_dentry->d_name.len);
- if (IS_ERR(wh_name)) {
+ if (unlikely(IS_ERR(wh_name))) {
err = PTR_ERR(wh_name);
goto out;
}
@@ -58,14 +58,14 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
lower_wh_dentry = lookup_one_len(wh_name, lower_new_dentry->d_parent,
new_dentry->d_name.len +
UNIONFS_WHLEN);
- if (IS_ERR(lower_wh_dentry)) {
+ if (unlikely(IS_ERR(lower_wh_dentry))) {
err = PTR_ERR(lower_wh_dentry);
goto out;
}
if (lower_wh_dentry->d_inode) {
/* get rid of the whiteout that is existing */
- if (lower_new_dentry->d_inode) {
+ if (unlikely(lower_new_dentry->d_inode)) {
printk(KERN_WARNING "unionfs: both a whiteout and a "
"dentry exist when doing a rename!\n");
err = -EIO;
@@ -81,7 +81,7 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
dput(lower_wh_dentry);
unlock_dir(lower_wh_dir_dentry);
- if (err)
+ if (unlikely(err))
goto out;
} else
dput(lower_wh_dentry);
@@ -93,7 +93,7 @@ static int ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/dentry.c | 68 ++++++++++++++++++++++++++------------------------
1 files changed, 35 insertions(+), 33 deletions(-)
diff --git a/fs/unionfs/dentry.c b/fs/unionfs/dentry.c
index 52bcb18..3f3a18d 100644
--- a/fs/unionfs/dentry.c
+++ b/fs/unionfs/dentry.c
@@ -45,7 +45,7 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
verify_locked(dentry);
/* if the dentry is unhashed, do NOT revalidate */
- if (d_deleted(dentry)) {
+ if (unlikely(d_deleted(dentry))) {
dprintk(KERN_DEBUG "unionfs: unhashed dentry being "
"revalidated: %*s\n",
dentry->d_name.len, dentry->d_name.name);
@@ -53,7 +53,7 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
}
BUG_ON(dbstart(dentry) == -1);
- if (dentry->d_inode)
+ if (likely(dentry->d_inode))
positive = 1;
dgen = atomic_read(&UNIONFS_D(dentry)->generation);
sbgen = atomic_read(&UNIONFS_SB(dentry->d_sb)->generation);
@@ -62,7 +62,7 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
* revalidation to be done, because this file does not exist within
* the namespace, and Unionfs operates on the namespace, not data.
*/
- if (sbgen != dgen) {
+ if (unlikely(sbgen != dgen)) {
struct dentry *result;
int pdgen;
@@ -76,7 +76,7 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
/* Free the pointers for our inodes and this dentry. */
bstart = dbstart(dentry);
bend = dbend(dentry);
- if (bstart >= 0) {
+ if (likely(bstart >= 0)) {
struct dentry *lower_dentry;
for (bindex = bstart; bindex <= bend; bindex++) {
lower_dentry =
@@ -89,7 +89,7 @@ static bool __unionfs_d_revalidate_one(struct dentry *dentry,
set_dbend(dentry, -1);
interpose_flag = INTERPOSE_REVAL_NEG;
- if (positive) {
+ if (likely(positive)) {
interpose_flag = INTERPOSE_REVAL;
/*
* During BRM, the VFS could already hold a lock on
@@ -97,14 +97,14 @@ static bool ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/copyup.c | 102 +++++++++++++++++++++++++-------------------------
1 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c
index 23ac4c8..e3c5f15 100644
--- a/fs/unionfs/copyup.c
+++ b/fs/unionfs/copyup.c
@@ -36,14 +36,14 @@ static int copyup_xattrs(struct dentry *old_lower_dentry,
/* query the actual size of the xattr list */
list_size = vfs_listxattr(old_lower_dentry, NULL, 0);
- if (list_size <= 0) {
+ if (unlikely(list_size <= 0)) {
err = list_size;
goto out;
}
/* allocate space for the actual list */
name_list = unionfs_xattr_alloc(list_size + 1, XATTR_LIST_MAX);
- if (!name_list || IS_ERR(name_list)) {
+ if (unlikely(!name_list || IS_ERR(name_list))) {
err = PTR_ERR(name_list);
goto out;
}
@@ -52,14 +52,14 @@ static int copyup_xattrs(struct dentry *old_lower_dentry,
/* now get the actual xattr list of the source file */
list_size = vfs_listxattr(old_lower_dentry, name_list, list_size);
- if (list_size <= 0) {
+ if (unlikely(list_size <= 0)) {
err = list_size;
goto out;
}
/* allocate space to hold each xattr's value */
attr_value = unionfs_xattr_alloc(XATTR_SIZE_MAX, XATTR_SIZE_MAX);
- if (!attr_value || IS_ERR(attr_value)) {
+ if (unlikely(!attr_value || IS_ERR(attr_value))) {
err = PTR_ERR(name_list);
goto out;
}
@@ -73,11 +73,11 @@ static int copyup_xattrs(struct dentry *old_lower_dentry,
size = vfs_getxattr(old_lower_dentry, name_list,
attr_value, XATTR_SIZE_MAX);
mutex_unlock(&old_lower_dentry->d_inode->i_mutex);
- if (size < 0) {
+ if (unlikely(size < 0)) {
err = size;
goto out;
}
- if (size > XATTR_SIZE_MAX) {
+ if (unlikely(size > XATTR_SIZE_MAX)) {
err = -E2BIG;
goto out;
}
@@ -91,13 +91,13 @@ static int copyup_xattrs(struct dentry *old_lower_dentry,
* temporarily get FOWNER privileges.
* XXX: move entire copyup code to ...I've been told several times that adding these is almost always bogus - either it messes up the CPU branch prediction or the compiler/CPU just does a lot better at finding the right way without these hints. Adding them as a blanket seems rather strange. Have you got any numbers that this really improves performance? Auke -
Auke, that's a good question, but I found it hard to find any info about it. There's no discussion on it in Documentation/, and very little I could find elsewhere. I did see one url explaining what un/likely does precisely, but no guidelines. My understanding is that it can improve performance, as long as it's used carefully (otherwise it may hurt performance). Background: we used un/likely in Unionfs only sparingly until now. We looked at what other filesystems and kernel components do, and it seems that it varies a lot: some subsystems use it religiously wherever they can, and some use it just a little here and there. We looked at what other filesystems do in particular and tried to follow a similar model under what cases other filesystems use un/likely. Recently we've done a full audit of the entire code, and added un/likely where we felt that the chance of succeeding is 95% or better (e.g., error conditions that should rarely happen, and such). So while it looks like we've added many of those in this series of patches, I can assure you we didn't just wrap every conditional in an un/likely just for the sake of using it. :-) There are plenty of conditionals (over 250) left untouched b/c it didn't make sense to force the branch prediction on them one way or another. So my questions are, for everyone, what's the policy on using un/likely? Any common conventions/wisdom? I think we need something added to Documentation/. Also, Auke, if indeed compilers are [sic] likely to do better than programmers adding un/likely wrappers, then why do we still support that in the kernel? (Working for a company tat produces high-quality compilers, you may know the answer better.) Personally I'm not too fond of what those wrappers do the code: they make it a bit harder to read the code (yet another extra set of parentheses); and they cause the code to be indented further to the right, which you sometimes have to split to multiple lines to avoid going over 80 ...
Actually due to the performance penalty on some systems I think you only want to use it if the chance of succeeding is 99% or better, as the benefit if predicted is a cycle or two and the harm if mispredicted can be more than 50 cycles, depending on the CPU. You should also remember than in filesystems many "failures" are triggered by things like the ld.so library searches, where it literally calls access() 20 different times on various possible paths for library files, failing the first 19. It does this once for each necessary library. Typically you only want to add unlikely() or likely() for about 2 reasons: (A) It's a hot path and the unlikely case is just going to burn a bunch of CPU anyways (B) It really is extremely unlikely that it fails (Think physical hardware failure) Anything else is just bogus. Cheers, Kyle Moffett -
*That's* the information I was looking for, Kyle: what's the estimated probability I should be using as my guideline. I used 95% (20/1 ratio), and you're telling me I should use 99% (100/1 ratio). The difference between the number of cycles saved/added is very compelling. Given that I certainly agree with you that I'm using un/likely too much. I'll re-evaluate and update my patch series then. Thanks, Erez. -
Yeah, close enough. :-) The important issue is that I'm probably using about five times too many un/likely wrappers. Erez. -
There are some places in generic code where it makes sense, e.g.:
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
If you run into a BUG() it's anyway game over.
And there are some rare hotpaths in the kernel where it might make
sense, and many other places where the likely/unlikely usage that might
be present doesn't make sense.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
-
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/debug.c | 108 +++++++++++++++++++++++++++------------------------
1 files changed, 57 insertions(+), 51 deletions(-)
diff --git a/fs/unionfs/debug.c b/fs/unionfs/debug.c
index 9546a41..09b52ce 100644
--- a/fs/unionfs/debug.c
+++ b/fs/unionfs/debug.c
@@ -56,19 +56,19 @@ void __unionfs_check_inode(const struct inode *inode,
sb = inode->i_sb;
istart = ibstart(inode);
iend = ibend(inode);
- if (istart > iend) {
+ if (unlikely(istart > iend)) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci0: inode=%p istart/end=%d:%d\n",
inode, istart, iend);
}
- if ((istart == -1 && iend != -1) ||
- (istart != -1 && iend == -1)) {
+ if (unlikely((istart == -1 && iend != -1) ||
+ (istart != -1 && iend == -1))) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci1: inode=%p istart/end=%d:%d\n",
inode, istart, iend);
}
if (!S_ISDIR(inode->i_mode)) {
- if (iend != istart) {
+ if (unlikely(iend != istart)) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci2: inode=%p istart=%d iend=%d\n",
inode, istart, iend);
@@ -76,24 +76,24 @@ void __unionfs_check_inode(const struct inode *inode,
}
for (bindex = sbstart(sb); bindex < sbmax(sb); bindex++) {
- if (!UNIONFS_I(inode)) {
+ if (unlikely(!UNIONFS_I(inode))) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci3: no inode_info %p\n", inode);
return;
}
- if (!UNIONFS_I(inode)->lower_inodes) {
+ if (unlikely(!UNIONFS_I(inode)->lower_inodes)) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci4: no lower_inodes %p\n", inode);
return;
}
lower_inode = unionfs_lower_inode_idx(inode, bindex);
if (lower_inode) {
- if (bindex < istart || bindex > iend) {
+ if (unlikely(bindex < istart || bindex > iend)) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci5: inode/linode=%p:%p bindex=%d "
"istart/end=%d:%d\n", inode,
lower_inode, bindex, istart, iend);
- } ...you could also do
if (unlikely((istart == -1 || iend == -1) && istart != iend)) {
[...]
and here
and here
[...]
-
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/commonfops.c | 94 +++++++++++++++++++++++-----------------------
1 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index e69ccf6..db8f064 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -64,7 +64,7 @@ retry:
tmp_dentry = lookup_one_len(name, lower_dentry->d_parent,
nlen);
- if (IS_ERR(tmp_dentry)) {
+ if (unlikely(IS_ERR(tmp_dentry))) {
err = PTR_ERR(tmp_dentry);
goto out;
}
@@ -73,8 +73,8 @@ retry:
err = copyup_named_file(dentry->d_parent->d_inode, file, name, bstart,
bindex, file->f_path.dentry->d_inode->i_size);
- if (err) {
- if (err == -EEXIST)
+ if (unlikely(err)) {
+ if (unlikely(err == -EEXIST))
goto retry;
goto out;
}
@@ -91,7 +91,7 @@ retry:
unlock_dir(lower_dir_dentry);
out:
- if (!err)
+ if (likely(!err))
unionfs_check_dentry(dentry);
return err;
}
@@ -126,7 +126,7 @@ static void cleanup_file(struct file *file)
*/
old_bid = UNIONFS_F(file)->saved_branch_ids[bindex];
i = branch_id_to_idx(sb, old_bid);
- if (i < 0) {
+ if (unlikely(i < 0)) {
printk(KERN_ERR "unionfs: no superblock for "
"file %p\n", file);
continue;
@@ -179,7 +179,7 @@ static int open_all_files(struct file *file)
dentry_open(lower_dentry,
unionfs_lower_mnt_idx(dentry, bindex),
file->f_flags);
- if (IS_ERR(lower_file)) {
+ if (unlikely(IS_ERR(lower_file))) {
err = PTR_ERR(lower_file);
goto out;
} else
@@ -208,7 +208,7 @@ static int open_highest_file(struct file *file, bool willwrite)
for (bindex = bstart - 1; bindex >= 0; bindex--) {
err = copyup_file(parent_inode, file, bstart, bindex,
inode_size);
- if (!err)
+ if (likely(!err))
break;
}
atomic_set(&UNIONFS_F(file)->generation,
@@ -222,7 +222,7 @@ static int open_highest_file(struct file *file, bool willwrite)
...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/inode.c | 160 ++++++++++++++++++++++++++--------------------------
1 files changed, 80 insertions(+), 80 deletions(-)
diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 7ee4760..7ae4a25 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -35,7 +35,7 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
unionfs_lock_dentry(dentry->d_parent);
valid = __unionfs_d_revalidate_chain(dentry->d_parent, nd, false);
unionfs_unlock_dentry(dentry->d_parent);
- if (!valid) {
+ if (unlikely(!valid)) {
err = -ESTALE; /* same as what real_lookup does */
goto out;
}
@@ -60,26 +60,26 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
* We _always_ create on branch 0
*/
lower_dentry = unionfs_lower_dentry_idx(dentry, 0);
- if (lower_dentry) {
+ if (likely(lower_dentry)) {
/*
* check if whiteout exists in this branch, i.e. lookup .wh.foo
* first.
*/
name = alloc_whname(dentry->d_name.name, dentry->d_name.len);
- if (IS_ERR(name)) {
+ if (unlikely(IS_ERR(name))) {
err = PTR_ERR(name);
goto out;
}
wh_dentry = lookup_one_len(name, lower_dentry->d_parent,
dentry->d_name.len + UNIONFS_WHLEN);
- if (IS_ERR(wh_dentry)) {
+ if (unlikely(IS_ERR(wh_dentry))) {
err = PTR_ERR(wh_dentry);
wh_dentry = NULL;
goto out;
}
- if (wh_dentry->d_inode) {
+ if (unlikely(wh_dentry->d_inode)) {
/*
* .wh.foo has been found, so let's unlink it
*/
@@ -89,7 +89,7 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
err = vfs_unlink(lower_dir_dentry->d_inode, wh_dentry);
unlock_dir(lower_dir_dentry);
- if (err) {
+ if (unlikely(err)) {
printk("unionfs_create: could not unlink "
"whiteout, err = %d\n", err);
goto out;
@@ -102,28 +102,28 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
*/
...From: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu> Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu> --- fs/unionfs/union.h | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/fs/unionfs/union.h b/fs/unionfs/union.h index 1cb2e1d..140b8ae 100644 --- a/fs/unionfs/union.h +++ b/fs/unionfs/union.h @@ -437,10 +437,6 @@ static inline int is_robranch(const struct dentry *dentry) #define UNIONFS_DIR_OPAQUE_NAME "__dir_opaque" #define UNIONFS_DIR_OPAQUE UNIONFS_WHPFX UNIONFS_DIR_OPAQUE_NAME -#ifndef DEFAULT_POLLMASK -#define DEFAULT_POLLMASK (POLLIN | POLLOUT | POLLRDNORM | POLLWRNORM) -#endif /* not DEFAULT_POLLMASK */ - /* * EXTERNALS: */ -- 1.5.2.2 -
From: Olivier Blin <blino@mandriva.com>
Do not update mtime if there is no upper branch for the inode. This
prevents from calling unionfs_lower_inode_idx() with a negative index, which
triggers a bug.
Signed-off-by: Olivier Blin <blino@mandriva.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/fanout.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/unionfs/fanout.h b/fs/unionfs/fanout.h
index afeb9f6..51aa0de 100644
--- a/fs/unionfs/fanout.h
+++ b/fs/unionfs/fanout.h
@@ -308,7 +308,7 @@ static inline void unionfs_copy_attr_times(struct inode *upper)
int bindex;
struct inode *lower;
- if (!upper)
+ if (!upper || ibstart(upper) < 0)
return;
for (bindex=ibstart(upper); bindex <= ibend(upper); bindex++) {
lower = unionfs_lower_inode_idx(upper, bindex);
--
1.5.2.2
-
From: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
Since we set the right value for d_type in readdir, there's really no point
in having to calculate the number of directory links. Some on-disk
filesystems don't even store the number of links for directories.
Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/subr.c | 41 +++++++----------------------------------
1 files changed, 7 insertions(+), 34 deletions(-)
diff --git a/fs/unionfs/subr.c b/fs/unionfs/subr.c
index b7e7904..6b93b64 100644
--- a/fs/unionfs/subr.c
+++ b/fs/unionfs/subr.c
@@ -188,16 +188,10 @@ out:
}
/*
- * returns the sum of the n_link values of all the underlying inodes of the
- * passed inode
+ * returns the right n_link value based on the inode type
*/
int unionfs_get_nlinks(const struct inode *inode)
{
- int sum_nlinks = 0;
- int dirs = 0;
- int bindex;
- struct inode *lower_inode;
-
/* don't bother to do all the work since we're unlinked */
if (inode->i_nlink == 0)
return 0;
@@ -205,33 +199,12 @@ int unionfs_get_nlinks(const struct inode *inode)
if (!S_ISDIR(inode->i_mode))
return unionfs_lower_inode(inode)->i_nlink;
- for (bindex = ibstart(inode); bindex <= ibend(inode); bindex++) {
- lower_inode = unionfs_lower_inode_idx(inode, bindex);
-
- /* ignore files */
- if (!lower_inode || !S_ISDIR(lower_inode->i_mode))
- continue;
-
- BUG_ON(lower_inode->i_nlink < 0);
-
- /* A deleted directory. */
- if (lower_inode->i_nlink == 0)
- continue;
- dirs++;
-
- /*
- * A broken directory...
- *
- * Some filesystems don't properly set the number of links
- * on empty directories
- */
- if (lower_inode->i_nlink == 1)
- sum_nlinks += 2;
- else
- sum_nlinks += (lower_inode->i_nlink - 2);
- }
-
- return (!dirs ? 0 : sum_nlinks + 2);
+ /*
+ * For directories, we return 1. The only place that could cares
+ * about links is readdir, and there's d_type there so even ...This also fixes a compile warning on 64-bit systems.
Signed-off-by: Josef 'Jeff' Sipek <jsipek@cs.sunysb.edu>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/debug.c | 16 ++++++----------
fs/unionfs/union.h | 1 +
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/fs/unionfs/debug.c b/fs/unionfs/debug.c
index 09b52ce..b103eb9 100644
--- a/fs/unionfs/debug.c
+++ b/fs/unionfs/debug.c
@@ -25,14 +25,6 @@
} \
} while (0)
-#if BITS_PER_LONG == 32
-#define POISONED_PTR ((void*) 0x5a5a5a5a)
-#elif BITS_PER_LONG == 64
-#define POISONED_PTR ((void*) 0x5a5a5a5a5a5a5a5a)
-#else
-#error Unknown BITS_PER_LONG value
-#endif /* BITS_PER_LONG != known */
-
/*
* __unionfs_check_{inode,dentry,file} perform exhaustive sanity checking on
* the fan-out of various Unionfs objects. We check that no lower objects
@@ -50,6 +42,7 @@ void __unionfs_check_inode(const struct inode *inode,
struct inode *lower_inode;
struct super_block *sb;
int printed_caller = 0;
+ void *poison_ptr;
/* for inodes now */
BUG_ON(!inode);
@@ -88,12 +81,13 @@ void __unionfs_check_inode(const struct inode *inode,
}
lower_inode = unionfs_lower_inode_idx(inode, bindex);
if (lower_inode) {
+ memset(&poison_ptr, POISON_INUSE, sizeof(void *));
if (unlikely(bindex < istart || bindex > iend)) {
PRINT_CALLER(fname, fxn, line);
printk(" Ci5: inode/linode=%p:%p bindex=%d "
"istart/end=%d:%d\n", inode,
lower_inode, bindex, istart, iend);
- } else if (unlikely(lower_inode == POISONED_PTR)) {
+ } else if (unlikely(lower_inode == poison_ptr)) {
/* freed inode! */
PRINT_CALLER(fname, fxn, line);
printk(" Ci6: inode/linode=%p:%p bindex=%d "
@@ -131,6 +125,7 @@ void __unionfs_check_dentry(const struct dentry *dentry,
struct super_block *sb;
struct vfsmount *lower_mnt;
int printed_caller = 0;
+ void *poison_ptr;
BUG_ON(!dentry);
sb = dentry->d_sb;
@@ -257,12 +252,13 @@ void ...Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
fs/unionfs/main.c | 98 ++++++++++++++++++++++++++-------------------------
fs/unionfs/super.c | 90 ++++++++++++++++++++++++------------------------
2 files changed, 95 insertions(+), 93 deletions(-)
diff --git a/fs/unionfs/main.c b/fs/unionfs/main.c
index 8595750..82cb35a 100644
--- a/fs/unionfs/main.c
+++ b/fs/unionfs/main.c
@@ -32,13 +32,13 @@ static void unionfs_fill_inode(struct dentry *dentry,
for (bindex = bstart; bindex <= bend; bindex++) {
lower_dentry = unionfs_lower_dentry_idx(dentry, bindex);
- if (!lower_dentry) {
+ if (unlikely(!lower_dentry)) {
unionfs_set_lower_inode_idx(inode, bindex, NULL);
continue;
}
/* Initialize the lower inode to the new lower inode. */
- if (!lower_dentry->d_inode)
+ if (unlikely(!lower_dentry->d_inode))
continue;
unionfs_set_lower_inode_idx(inode, bindex,
@@ -52,7 +52,7 @@ static void unionfs_fill_inode(struct dentry *dentry,
lower_inode = unionfs_lower_inode(inode);
/* Use different set of inode ops for symlinks & directories */
- if (S_ISLNK(lower_inode->i_mode))
+ if (unlikely(S_ISLNK(lower_inode->i_mode)))
inode->i_op = &unionfs_symlink_iops;
else if (S_ISDIR(lower_inode->i_mode))
inode->i_op = &unionfs_dir_iops;
@@ -62,8 +62,10 @@ static void unionfs_fill_inode(struct dentry *dentry,
inode->i_fop = &unionfs_dir_fops;
/* properly initialize special inodes */
- if (S_ISBLK(lower_inode->i_mode) || S_ISCHR(lower_inode->i_mode) ||
- S_ISFIFO(lower_inode->i_mode) || S_ISSOCK(lower_inode->i_mode))
+ if (unlikely(S_ISBLK(lower_inode->i_mode) ||
+ S_ISCHR(lower_inode->i_mode) ||
+ S_ISFIFO(lower_inode->i_mode) ||
+ S_ISSOCK(lower_inode->i_mode)))
init_special_inode(inode, lower_inode->i_mode,
lower_inode->i_rdev);
@@ -122,14 +124,14 @@ struct dentry *unionfs_interpose(struct dentry *dentry, struct super_block *sb,
UNIONFS_I(inode)->lower_inodes =
kcalloc(sbmax(sb), ...
