| Message ID | 1408804878-1331-5-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 23, 2014 at 10:41:12AM -0400, Jeff Layton wrote: > Some of the latter paragraphs seem ambiguous and just plain wrong. > In particular the break_lease comment makes no sense. We call > break_lease (and break_deleg) from all sorts of vfs-layer functions, > so there is clearly such a method. > > Also, we are close to being able to allow for "real" filesystem > setlease methods so remove the final comment about it not being a > full implementation yet. I'd remove even more: > + * > + * This will call the filesystem's setlease file method, if defined. Note that > + * there is no getlease method; instead, the filesystem setlease method should > + * call back to generic_setlease() to add a lease to the inode's lease list, > + * where fcntl_getlease() can find it. Since fcntl_getlease() only reports > + * whether the current task holds a lease, a cluster filesystem need only do > + * this for leases held by processes on this node. > */ If we'd ever want a full implementation I think we'd absolutely need the getlease method. But instead of hypothetizing about future implementation I'd rather leave it to those actually implementing such support, if that ever happens. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 24, 2014 at 08:51:17AM -0700, Christoph Hellwig wrote: > On Sat, Aug 23, 2014 at 10:41:12AM -0400, Jeff Layton wrote: > > Some of the latter paragraphs seem ambiguous and just plain wrong. > > In particular the break_lease comment makes no sense. We call > > break_lease (and break_deleg) from all sorts of vfs-layer functions, > > so there is clearly such a method. Right, but there's no f_op->break_lease. Anyway: > > Also, we are close to being able to allow for "real" filesystem > > setlease methods so remove the final comment about it not being a > > full implementation yet. > > I'd remove even more: > > > + * > > + * This will call the filesystem's setlease file method, if defined. Note that > > + * there is no getlease method; instead, the filesystem setlease method should > > + * call back to generic_setlease() to add a lease to the inode's lease list, > > + * where fcntl_getlease() can find it. Since fcntl_getlease() only reports > > + * whether the current task holds a lease, a cluster filesystem need only do > > + * this for leases held by processes on this node. > > */ > > If we'd ever want a full implementation I think we'd absolutely need > the getlease method. But instead of hypothetizing about future > implementation I'd rather leave it to those actually implementing such > support, if that ever happens. I agree, that makes sense. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" 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/locks.c b/fs/locks.c index bedb817a5cc4..597e71a1e90f 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1705,30 +1705,23 @@ static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease) } /** - * vfs_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @lease: file_lock to use - * - * Call this to establish a lease on the file. - * The (*lease)->fl_lmops->lm_break operation must be set; if not, - * break_lease will oops! - * - * This will call the filesystem's setlease file method, if - * defined. Note that there is no getlease method; instead, the - * filesystem setlease method should call back to setlease() to - * add a lease to the inode's lease list, where fcntl_getlease() can - * find it. Since fcntl_getlease() only reports whether the current - * task holds a lease, a cluster filesystem need only do this for - * leases held by processes on this node. - * - * There is also no break_lease method; filesystems that - * handle their own leases should break leases themselves from the - * filesystem's open, create, and (on truncate) setattr methods. - * - * Warning: the only current setlease methods exist only to disable - * leases in certain cases. More vfs changes may be required to - * allow a full filesystem lease implementation. + * vfs_setlease - sets a lease on an open file + * @filp: file pointer + * @arg: type of lease to obtain + * @lease: file_lock to use when adding a lease + * + * Call this to establish a lease on the file. The "lease" argument is not + * used for F_UNLCK requests and may be NULL. For commands that set or alter + * an existing lease, the (*lease)->fl_lmops->lm_break operation must be set; + * if not, this function will return -EINVAL (and generate a scary-looking + * stack trace). + * + * This will call the filesystem's setlease file method, if defined. Note that + * there is no getlease method; instead, the filesystem setlease method should + * call back to generic_setlease() to add a lease to the inode's lease list, + * where fcntl_getlease() can find it. Since fcntl_getlease() only reports + * whether the current task holds a lease, a cluster filesystem need only do + * this for leases held by processes on this node. */ int vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
Some of the latter paragraphs seem ambiguous and just plain wrong. In particular the break_lease comment makes no sense. We call break_lease (and break_deleg) from all sorts of vfs-layer functions, so there is clearly such a method. Also, we are close to being able to allow for "real" filesystem setlease methods so remove the final comment about it not being a full implementation yet. Signed-off-by: Jeff Layton <jlayton@primarydata.com> --- fs/locks.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)