From 130d1f956ab367bab855336279afa3b19acdc9a1 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 9 May 2014 14:13:04 -0400 Subject: [PATCH 1/3] locks: ensure that fl_owner is always initialized properly in flock and lease codepaths Currently, the fl_owner isn't set for flock locks. Some filesystems use byte-range locks to simulate flock locks and there is a common idiom in those that does: fl->fl_owner = (fl_owner_t)filp; fl->fl_start = 0; fl->fl_end = OFFSET_MAX; Since flock locks are generally "owned" by the open file description, move this into the common flock lock setup code. The fl_start and fl_end fields are already set appropriately, so remove the unneeded setting of that in flock ops in those filesystems as well. Finally, the lease code also sets the fl_owner as if they were owned by the process and not the open file description. This is incorrect as leases have the same ownership semantics as flock locks. Set them the same way. The lease code doesn't actually use the fl_owner value for anything, so this is more for consistency's sake than a bugfix. Reported-by: Trond Myklebust Signed-off-by: Jeff Layton Acked-by: Greg Kroah-Hartman (Staging portion) Acked-by: J. Bruce Fields --- drivers/staging/lustre/lustre/llite/file.c | 17 ++++++----------- fs/9p/vfs_file.c | 3 --- fs/afs/flock.c | 4 ---- fs/ceph/locks.c | 10 ++-------- fs/fuse/file.c | 1 - fs/locks.c | 4 +++- fs/nfs/file.c | 4 ---- 7 files changed, 11 insertions(+), 32 deletions(-) diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c index 8e844a6371e0..760ccd83f1f7 100644 --- a/drivers/staging/lustre/lustre/llite/file.c +++ b/drivers/staging/lustre/lustre/llite/file.c @@ -2691,20 +2691,15 @@ int ll_file_flock(struct file *file, int cmd, struct file_lock *file_lock) ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_FLOCK, 1); - if (file_lock->fl_flags & FL_FLOCK) { + if (file_lock->fl_flags & FL_FLOCK) LASSERT((cmd == F_SETLKW) || (cmd == F_SETLK)); - /* flocks are whole-file locks */ - flock.l_flock.end = OFFSET_MAX; - /* For flocks owner is determined by the local file descriptor*/ - flock.l_flock.owner = (unsigned long)file_lock->fl_file; - } else if (file_lock->fl_flags & FL_POSIX) { - flock.l_flock.owner = (unsigned long)file_lock->fl_owner; - flock.l_flock.start = file_lock->fl_start; - flock.l_flock.end = file_lock->fl_end; - } else { + else if (!(file_lock->fl_flags & FL_POSIX)) return -EINVAL; - } + + flock.l_flock.owner = (unsigned long)file_lock->fl_owner; flock.l_flock.pid = file_lock->fl_pid; + flock.l_flock.start = file_lock->fl_start; + flock.l_flock.end = file_lock->fl_end; /* Somewhat ugly workaround for svc lockd. * lockd installs custom fl_lmops->lm_compare_owner that checks diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c index d8223209d4b1..59e3fe3d56c0 100644 --- a/fs/9p/vfs_file.c +++ b/fs/9p/vfs_file.c @@ -352,9 +352,6 @@ static int v9fs_file_flock_dotl(struct file *filp, int cmd, invalidate_mapping_pages(&inode->i_data, 0, -1); } /* Convert flock to posix lock */ - fl->fl_owner = (fl_owner_t)filp; - fl->fl_start = 0; - fl->fl_end = OFFSET_MAX; fl->fl_flags |= FL_POSIX; fl->fl_flags ^= FL_FLOCK; diff --git a/fs/afs/flock.c b/fs/afs/flock.c index a8cf2cff836c..4baf1d2b39e4 100644 --- a/fs/afs/flock.c +++ b/fs/afs/flock.c @@ -555,10 +555,6 @@ int afs_flock(struct file *file, int cmd, struct file_lock *fl) return -ENOLCK; /* we're simulating flock() locks using posix locks on the server */ - fl->fl_owner = (fl_owner_t) file; - fl->fl_start = 0; - fl->fl_end = OFFSET_MAX; - if (fl->fl_type == F_UNLCK) return afs_do_unlk(file, fl); return afs_do_setlk(file, fl); diff --git a/fs/ceph/locks.c b/fs/ceph/locks.c index 191398852a2e..fbc39c47bacd 100644 --- a/fs/ceph/locks.c +++ b/fs/ceph/locks.c @@ -53,10 +53,7 @@ static int ceph_lock_message(u8 lock_type, u16 operation, struct file *file, else length = fl->fl_end - fl->fl_start + 1; - if (lock_type == CEPH_LOCK_FCNTL) - owner = secure_addr(fl->fl_owner); - else - owner = secure_addr(fl->fl_file); + owner = secure_addr(fl->fl_owner); dout("ceph_lock_message: rule: %d, op: %d, owner: %llx, pid: %llu, " "start: %llu, length: %llu, wait: %d, type: %d", (int)lock_type, @@ -314,10 +311,7 @@ int lock_to_ceph_filelock(struct file_lock *lock, cephlock->length = cpu_to_le64(lock->fl_end - lock->fl_start + 1); cephlock->client = cpu_to_le64(0); cephlock->pid = cpu_to_le64((u64)lock->fl_pid); - if (lock->fl_flags & FL_POSIX) - cephlock->owner = cpu_to_le64(secure_addr(lock->fl_owner)); - else - cephlock->owner = cpu_to_le64(secure_addr(lock->fl_file)); + cephlock->owner = cpu_to_le64(secure_addr(lock->fl_owner)); switch (lock->fl_type) { case F_RDLCK: diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 96d513e01a5d..f680d2c44e97 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -2304,7 +2304,6 @@ static int fuse_file_flock(struct file *file, int cmd, struct file_lock *fl) struct fuse_file *ff = file->private_data; /* emulate flock with POSIX locks */ - fl->fl_owner = (fl_owner_t) file; ff->flock = true; err = fuse_setlk(file, fl, 1); } diff --git a/fs/locks.c b/fs/locks.c index e390bd9ae068..77aff897fef3 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -322,6 +322,7 @@ static int flock_make_lock(struct file *filp, struct file_lock **lock, return -ENOMEM; fl->fl_file = filp; + fl->fl_owner = (fl_owner_t)filp; fl->fl_pid = current->tgid; fl->fl_flags = FL_FLOCK; fl->fl_type = type; @@ -427,7 +428,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl) if (assign_type(fl, type) != 0) return -EINVAL; - fl->fl_owner = current->files; + fl->fl_owner = (fl_owner_t)filp; fl->fl_pid = current->tgid; fl->fl_file = filp; @@ -2316,6 +2317,7 @@ void locks_remove_file(struct file *filp) if (filp->f_op->flock) { struct file_lock fl = { + .fl_owner = (fl_owner_t)filp, .fl_pid = current->tgid, .fl_file = filp, .fl_flags = FL_FLOCK, diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 284ca901fe16..c1edf7336315 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -916,10 +916,6 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) is_local = 1; /* We're simulating flock() locks using posix locks on the server */ - fl->fl_owner = (fl_owner_t)filp; - fl->fl_start = 0; - fl->fl_end = OFFSET_MAX; - if (fl->fl_type == F_UNLCK) return do_unlk(filp, cmd, fl, is_local); return do_setlk(filp, cmd, fl, is_local); From 5315c26a6c557f44733725004fc7dc25c8a94cd5 Mon Sep 17 00:00:00 2001 From: Fabian Frederick Date: Fri, 9 May 2014 14:13:05 -0400 Subject: [PATCH 2/3] fs/locks.c: replace seq_printf by seq_puts Replace seq_printf where possible Cc: Jeff Layton Cc: Alexander Viro Cc: Andrew Morton Signed-off-by: Fabian Frederick Signed-off-by: Jeff Layton --- fs/locks.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 77aff897fef3..facf76d9fcb4 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -2425,31 +2425,31 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, seq_printf(f, "%lld:%s ", id, pfx); if (IS_POSIX(fl)) { if (fl->fl_flags & FL_ACCESS) - seq_printf(f, "ACCESS"); + seq_puts(f, "ACCESS"); else if (IS_OFDLCK(fl)) - seq_printf(f, "OFDLCK"); + seq_puts(f, "OFDLCK"); else - seq_printf(f, "POSIX "); + seq_puts(f, "POSIX "); seq_printf(f, " %s ", (inode == NULL) ? "*NOINODE*" : mandatory_lock(inode) ? "MANDATORY" : "ADVISORY "); } else if (IS_FLOCK(fl)) { if (fl->fl_type & LOCK_MAND) { - seq_printf(f, "FLOCK MSNFS "); + seq_puts(f, "FLOCK MSNFS "); } else { - seq_printf(f, "FLOCK ADVISORY "); + seq_puts(f, "FLOCK ADVISORY "); } } else if (IS_LEASE(fl)) { - seq_printf(f, "LEASE "); + seq_puts(f, "LEASE "); if (lease_breaking(fl)) - seq_printf(f, "BREAKING "); + seq_puts(f, "BREAKING "); else if (fl->fl_file) - seq_printf(f, "ACTIVE "); + seq_puts(f, "ACTIVE "); else - seq_printf(f, "BREAKER "); + seq_puts(f, "BREAKER "); } else { - seq_printf(f, "UNKNOWN UNKNOWN "); + seq_puts(f, "UNKNOWN UNKNOWN "); } if (fl->fl_type & LOCK_MAND) { seq_printf(f, "%s ", @@ -2481,7 +2481,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, else seq_printf(f, "%Ld %Ld\n", fl->fl_start, fl->fl_end); } else { - seq_printf(f, "0 EOF\n"); + seq_puts(f, "0 EOF\n"); } } From 62af4f1f7df44ea0bb1a11c94ac9fb384bf1c564 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Fri, 9 May 2014 14:13:05 -0400 Subject: [PATCH 3/3] locks: add some tracepoints in the lease handling code v2: add a __break_lease tracepoint for non-blocking case Recently, I needed these to help track down a softlockup when recalling a delegation, but they might be helpful in other situations as well. Cc: "J. Bruce Fields" Signed-off-by: Jeff Layton --- fs/locks.c | 11 ++++ include/trace/events/filelock.h | 96 +++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 include/trace/events/filelock.h diff --git a/fs/locks.c b/fs/locks.c index facf76d9fcb4..da57c9b7e844 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -130,6 +130,9 @@ #include #include +#define CREATE_TRACE_POINTS +#include + #include #define IS_POSIX(fl) (fl->fl_flags & FL_POSIX) @@ -1287,6 +1290,7 @@ static void time_out_leases(struct inode *inode) before = &inode->i_flock; while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) { + trace_time_out_leases(inode, fl); if (past_time(fl->fl_downgrade_time)) lease_modify(before, F_RDLCK); if (past_time(fl->fl_break_time)) @@ -1374,6 +1378,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) } if (i_have_this_lease || (mode & O_NONBLOCK)) { + trace_break_lease_noblock(inode, new_fl); error = -EWOULDBLOCK; goto out; } @@ -1385,10 +1390,12 @@ restart: if (break_time == 0) break_time++; locks_insert_block(flock, new_fl); + trace_break_lease_block(inode, new_fl); spin_unlock(&inode->i_lock); error = wait_event_interruptible_timeout(new_fl->fl_wait, !new_fl->fl_next, break_time); spin_lock(&inode->i_lock); + trace_break_lease_unblock(inode, new_fl); locks_delete_block(new_fl); if (error >= 0) { if (error == 0) @@ -1510,6 +1517,8 @@ static int generic_add_lease(struct file *filp, long arg, struct file_lock **flp int error; lease = *flp; + trace_generic_add_lease(inode, lease); + /* * In the delegation case we need mutual exclusion with * a number of operations that take the i_mutex. We trylock @@ -1599,6 +1608,8 @@ static int generic_delete_lease(struct file *filp, struct file_lock **flp) struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; + trace_generic_delete_lease(inode, *flp); + for (before = &inode->i_flock; ((fl = *before) != NULL) && IS_LEASE(fl); before = &fl->fl_next) { diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h new file mode 100644 index 000000000000..59d11c22f076 --- /dev/null +++ b/include/trace/events/filelock.h @@ -0,0 +1,96 @@ +/* + * Events for filesystem locks + * + * Copyright 2013 Jeff Layton + */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM filelock + +#if !defined(_TRACE_FILELOCK_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_FILELOCK_H + +#include +#include +#include +#include + +#define show_fl_flags(val) \ + __print_flags(val, "|", \ + { FL_POSIX, "FL_POSIX" }, \ + { FL_FLOCK, "FL_FLOCK" }, \ + { FL_DELEG, "FL_DELEG" }, \ + { FL_ACCESS, "FL_ACCESS" }, \ + { FL_EXISTS, "FL_EXISTS" }, \ + { FL_LEASE, "FL_LEASE" }, \ + { FL_CLOSE, "FL_CLOSE" }, \ + { FL_SLEEP, "FL_SLEEP" }, \ + { FL_DOWNGRADE_PENDING, "FL_DOWNGRADE_PENDING" }, \ + { FL_UNLOCK_PENDING, "FL_UNLOCK_PENDING" }, \ + { FL_OFDLCK, "FL_OFDLCK" }) + +#define show_fl_type(val) \ + __print_symbolic(val, \ + { F_RDLCK, "F_RDLCK" }, \ + { F_WRLCK, "F_WRLCK" }, \ + { F_UNLCK, "F_UNLCK" }) + +DECLARE_EVENT_CLASS(filelock_lease, + + TP_PROTO(struct inode *inode, struct file_lock *fl), + + TP_ARGS(inode, fl), + + TP_STRUCT__entry( + __field(struct file_lock *, fl) + __field(unsigned long, i_ino) + __field(dev_t, s_dev) + __field(struct file_lock *, fl_next) + __field(fl_owner_t, fl_owner) + __field(unsigned int, fl_flags) + __field(unsigned char, fl_type) + __field(unsigned long, fl_break_time) + __field(unsigned long, fl_downgrade_time) + ), + + TP_fast_assign( + __entry->fl = fl; + __entry->s_dev = inode->i_sb->s_dev; + __entry->i_ino = inode->i_ino; + __entry->fl_next = fl->fl_next; + __entry->fl_owner = fl->fl_owner; + __entry->fl_flags = fl->fl_flags; + __entry->fl_type = fl->fl_type; + __entry->fl_break_time = fl->fl_break_time; + __entry->fl_downgrade_time = fl->fl_downgrade_time; + ), + + TP_printk("fl=0x%p dev=0x%x:0x%x ino=0x%lx fl_next=0x%p fl_owner=0x%p fl_flags=%s fl_type=%s fl_break_time=%lu fl_downgrade_time=%lu", + __entry->fl, MAJOR(__entry->s_dev), MINOR(__entry->s_dev), + __entry->i_ino, __entry->fl_next, __entry->fl_owner, + show_fl_flags(__entry->fl_flags), + show_fl_type(__entry->fl_type), + __entry->fl_break_time, __entry->fl_downgrade_time) +); + +DEFINE_EVENT(filelock_lease, break_lease_noblock, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +DEFINE_EVENT(filelock_lease, break_lease_block, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +DEFINE_EVENT(filelock_lease, break_lease_unblock, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +DEFINE_EVENT(filelock_lease, generic_add_lease, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +DEFINE_EVENT(filelock_lease, generic_delete_lease, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +DEFINE_EVENT(filelock_lease, time_out_leases, TP_PROTO(struct inode *inode, struct file_lock *fl), + TP_ARGS(inode, fl)); + +#endif /* _TRACE_FILELOCK_H */ + +/* This part must be outside protection */ +#include