* [PATCH 00/10] Eliminate delegation self-conflicts v2
@ 2018-03-19 14:36 J. Bruce Fields
2018-03-19 14:36 ` [PATCH 01/10] vfs: remove unnecessary fl_owner_t typedef J. Bruce Fields
` (11 more replies)
0 siblings, 12 replies; 19+ messages in thread
From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw)
To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
This is my second attempt to fix the NFS server so we don't
unnecessarily recall delegations when the operation breaking the
delegation comes from the same client that holds the delegation.
To do that we need some way to pass the identity of the breaker down
through the VFS. In my first attempt I tried passing that explicitly,
but that touches a lot of code. So instead I'm experimenting with
adding a field to the cred struct.
Testing has confirmed that this works. (And that the pynfs tests are
broken--they *require* the server to break delegations on operations
coming from the client, even though that's the less desireable behavior.
I'm fixing that...).
J. Bruce Fields (10):
vfs: remove unnecessary fl_owner_t typedef
nfsd: simplify put of fi_deleg_file
nfsd: simplify nfs4_put_deleg_lease calls
nfsd4: set fl_owner to delegation, not file pointer
nfsd4: dp->dl_stid.sc_file doesn't need locking
nfsd: make nfs4_get_existing_delegation less confusing
nfsd: factor out common delegation-destruction code
nfsd: move sc_file assignment into alloc_init_deleg
nfsd: create a separate lease for each delegation
nfsd: clients don't need to break their own delegations
Documentation/filesystems/Locking | 2 +
Documentation/filesystems/vfs.txt | 2 +-
arch/ia64/kernel/perfmon.c | 2 +-
arch/powerpc/platforms/cell/spufs/file.c | 2 +-
arch/tile/kernel/hardwall.c | 2 +-
drivers/android/binder.c | 2 +-
drivers/char/ps3flash.c | 2 +-
drivers/char/xillybus/xillybus_core.c | 2 +-
drivers/firmware/efi/capsule-loader.c | 2 +-
drivers/input/evdev.c | 2 +-
drivers/misc/mic/scif/scif_fd.c | 2 +-
drivers/scsi/osst.c | 2 +-
drivers/scsi/st.c | 2 +-
drivers/staging/lustre/lustre/llite/file.c | 2 +-
drivers/usb/class/cdc-wdm.c | 2 +-
drivers/usb/usb-skeleton.c | 2 +-
fs/afs/internal.h | 2 +-
fs/afs/write.c | 2 +-
fs/cifs/cifsfs.h | 2 +-
fs/cifs/file.c | 4 +-
fs/ecryptfs/file.c | 2 +-
fs/exofs/file.c | 2 +-
fs/f2fs/file.c | 2 +-
fs/fuse/file.c | 13 +-
fs/fuse/fuse_i.h | 2 +-
fs/lockd/clntproc.c | 4 +-
fs/lockd/svc4proc.c | 2 +-
fs/lockd/svcproc.c | 2 +-
fs/locks.c | 6 +-
fs/nfs/file.c | 2 +-
fs/nfs/inode.c | 2 +-
fs/nfs/nfs4_fs.h | 2 +-
fs/nfs/nfs4file.c | 2 +-
fs/nfs/nfs4state.c | 8 +-
fs/nfsd/auth.c | 2 +
fs/nfsd/nfs4proc.c | 1 +
fs/nfsd/nfs4state.c | 267 ++++++++++++-----------------
fs/nfsd/nfssvc.c | 1 +
fs/notify/dnotify/dnotify.c | 6 +-
fs/open.c | 2 +-
include/linux/cred.h | 3 +
include/linux/dnotify.h | 6 +-
include/linux/fs.h | 18 +-
include/linux/lockd/lockd.h | 4 +-
include/linux/nfs_fs.h | 4 +-
include/linux/sunrpc/svc.h | 1 +
include/trace/events/filelock.h | 6 +-
ipc/mqueue.c | 2 +-
48 files changed, 193 insertions(+), 223 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 01/10] vfs: remove unnecessary fl_owner_t typedef 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 02/10] nfsd: simplify put of fi_deleg_file J. Bruce Fields ` (10 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> The convention is to avoid this kind of typedef. It doesn't seem useful, and it requires a lot of casts. Reviewed-by: NeilBrown <neilb@suse.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- Documentation/filesystems/vfs.txt | 2 +- arch/ia64/kernel/perfmon.c | 2 +- arch/powerpc/platforms/cell/spufs/file.c | 2 +- arch/tile/kernel/hardwall.c | 2 +- drivers/android/binder.c | 2 +- drivers/char/ps3flash.c | 2 +- drivers/char/xillybus/xillybus_core.c | 2 +- drivers/firmware/efi/capsule-loader.c | 2 +- drivers/input/evdev.c | 2 +- drivers/misc/mic/scif/scif_fd.c | 2 +- drivers/scsi/osst.c | 2 +- drivers/scsi/st.c | 2 +- drivers/staging/lustre/lustre/llite/file.c | 2 +- drivers/usb/class/cdc-wdm.c | 2 +- drivers/usb/usb-skeleton.c | 2 +- fs/afs/internal.h | 2 +- fs/afs/write.c | 2 +- fs/cifs/cifsfs.h | 2 +- fs/cifs/file.c | 4 ++-- fs/ecryptfs/file.c | 2 +- fs/exofs/file.c | 2 +- fs/f2fs/file.c | 2 +- fs/fuse/file.c | 13 ++++++------- fs/fuse/fuse_i.h | 2 +- fs/lockd/clntproc.c | 4 ++-- fs/lockd/svc4proc.c | 2 +- fs/lockd/svcproc.c | 2 +- fs/locks.c | 2 +- fs/nfs/file.c | 2 +- fs/nfs/inode.c | 2 +- fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4file.c | 2 +- fs/nfs/nfs4state.c | 8 ++++---- fs/nfsd/nfs4state.c | 18 +++++++++--------- fs/notify/dnotify/dnotify.c | 6 +++--- fs/open.c | 2 +- include/linux/dnotify.h | 6 +++--- include/linux/fs.h | 17 +++++++---------- include/linux/lockd/lockd.h | 4 ++-- include/linux/nfs_fs.h | 4 ++-- include/trace/events/filelock.h | 6 +++--- ipc/mqueue.c | 2 +- 42 files changed, 74 insertions(+), 78 deletions(-) diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 5fd325df59e2..7e22079c7396 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -862,7 +862,7 @@ struct file_operations { int (*mmap) (struct file *, struct vm_area_struct *); int (*mremap)(struct file *, struct vm_area_struct *); int (*open) (struct inode *, struct file *); - int (*flush) (struct file *, fl_owner_t id); + int (*flush) (struct file *, void *id); int (*release) (struct inode *, struct file *); int (*fsync) (struct file *, loff_t, loff_t, int datasync); int (*fasync) (int, struct file *, int); diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c index 8fb280e33114..c78e62a76d20 100644 --- a/arch/ia64/kernel/perfmon.c +++ b/arch/ia64/kernel/perfmon.c @@ -1809,7 +1809,7 @@ pfm_syswide_cleanup_other_cpu(pfm_context_t *ctx) * When caller is self-monitoring, the context is unloaded. */ static int -pfm_flush(struct file *filp, fl_owner_t id) +pfm_flush(struct file *filp, void *id) { pfm_context_t *ctx; struct task_struct *task; diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c index 469bdd0b748f..e5a204f8169b 100644 --- a/arch/powerpc/platforms/cell/spufs/file.c +++ b/arch/powerpc/platforms/cell/spufs/file.c @@ -1720,7 +1720,7 @@ static __poll_t spufs_mfc_poll(struct file *file,poll_table *wait) return mask; } -static int spufs_mfc_flush(struct file *file, fl_owner_t id) +static int spufs_mfc_flush(struct file *file, void *id) { struct spu_context *ctx = file->private_data; int ret; diff --git a/arch/tile/kernel/hardwall.c b/arch/tile/kernel/hardwall.c index 2fd1694ac1d0..b2cf21d1edb0 100644 --- a/arch/tile/kernel/hardwall.c +++ b/arch/tile/kernel/hardwall.c @@ -1030,7 +1030,7 @@ static long hardwall_compat_ioctl(struct file *file, #endif /* The user process closed the file; revoke access to user networks. */ -static int hardwall_flush(struct file *file, fl_owner_t owner) +static int hardwall_flush(struct file *file, void *owner) { struct hardwall_info *info = file->private_data; struct task_struct *task, *tmp; diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 15e3d3c2260d..f7706142974c 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -4776,7 +4776,7 @@ static int binder_open(struct inode *nodp, struct file *filp) return 0; } -static int binder_flush(struct file *filp, fl_owner_t id) +static int binder_flush(struct file *filp, void *id) { struct binder_proc *proc = filp->private_data; diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c index b526dc15c271..5a03dd0eb2f1 100644 --- a/drivers/char/ps3flash.c +++ b/drivers/char/ps3flash.c @@ -281,7 +281,7 @@ static ssize_t ps3flash_kernel_write(const void *buf, size_t count, return res; } -static int ps3flash_flush(struct file *file, fl_owner_t id) +static int ps3flash_flush(struct file *file, void *id) { return ps3flash_writeback(ps3flash_dev); } diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c index a11af94e2e65..b76074c976b9 100644 --- a/drivers/char/xillybus/xillybus_core.c +++ b/drivers/char/xillybus/xillybus_core.c @@ -1156,7 +1156,7 @@ static int xillybus_myflush(struct xilly_channel *channel, long timeout) return rc; } -static int xillybus_flush(struct file *filp, fl_owner_t id) +static int xillybus_flush(struct file *filp, void *id) { if (!(filp->f_mode & FMODE_WRITE)) return 0; diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c index e456f4602df1..ba444f726465 100644 --- a/drivers/firmware/efi/capsule-loader.c +++ b/drivers/firmware/efi/capsule-loader.c @@ -246,7 +246,7 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff, * will be treated as upload termination and will free those completed * buffer pages and -ECANCELED will be returned. **/ -static int efi_capsule_flush(struct file *file, fl_owner_t id) +static int efi_capsule_flush(struct file *file, void *id) { int ret = 0; struct capsule_info *cap_info = file->private_data; diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index c81c79d01d93..fd9e02e133b1 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -348,7 +348,7 @@ static int evdev_fasync(int fd, struct file *file, int on) return fasync_helper(fd, file, on, &client->fasync); } -static int evdev_flush(struct file *file, fl_owner_t id) +static int evdev_flush(struct file *file, void *id) { struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev; diff --git a/drivers/misc/mic/scif/scif_fd.c b/drivers/misc/mic/scif/scif_fd.c index 5c2a57ae4f85..e8baa7b93c05 100644 --- a/drivers/misc/mic/scif/scif_fd.c +++ b/drivers/misc/mic/scif/scif_fd.c @@ -48,7 +48,7 @@ static __poll_t scif_fdpoll(struct file *f, poll_table *wait) return __scif_pollfd(f, wait, priv); } -static int scif_fdflush(struct file *f, fl_owner_t id) +static int scif_fdflush(struct file *f, void *id) { struct scif_endpt *ep = f->private_data; diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c index 20ec1c01dbd5..fdb213261249 100644 --- a/drivers/scsi/osst.c +++ b/drivers/scsi/osst.c @@ -4822,7 +4822,7 @@ static int os_scsi_tape_open(struct inode * inode, struct file * filp) /* Flush the tape buffer before close */ -static int os_scsi_tape_flush(struct file * filp, fl_owner_t id) +static int os_scsi_tape_flush(struct file * filp, void *id) { int result = 0, result2; struct osst_tape * STp = filp->private_data; diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c index 6c399480783d..ed01299f6f51 100644 --- a/drivers/scsi/st.c +++ b/drivers/scsi/st.c @@ -1338,7 +1338,7 @@ static int st_open(struct inode *inode, struct file *filp) \f /* Flush the tape buffer before close */ -static int st_flush(struct file *filp, fl_owner_t id) +static int st_flush(struct file *filp, void *id) { int result = 0, result2; unsigned char cmd[MAX_COMMAND_SIZE]; diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c index 938b859b6650..a8fe2072ea7c 100644 --- a/drivers/staging/lustre/lustre/llite/file.c +++ b/drivers/staging/lustre/lustre/llite/file.c @@ -2281,7 +2281,7 @@ static loff_t ll_file_seek(struct file *file, loff_t offset, int origin) ll_file_maxbytes(inode), eof); } -static int ll_flush(struct file *file, fl_owner_t id) +static int ll_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); struct ll_inode_info *lli = ll_i2info(inode); diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index a0d284ef3f40..bda5b9812ccb 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -581,7 +581,7 @@ static ssize_t wdm_read return rv; } -static int wdm_flush(struct file *file, fl_owner_t id) +static int wdm_flush(struct file *file, void *id) { struct wdm_device *desc = file->private_data; diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c index 26ca0ec01fd5..d20a23d77fbe 100644 --- a/drivers/usb/usb-skeleton.c +++ b/drivers/usb/usb-skeleton.c @@ -132,7 +132,7 @@ static int skel_release(struct inode *inode, struct file *file) return 0; } -static int skel_flush(struct file *file, fl_owner_t id) +static int skel_flush(struct file *file, void *id) { struct usb_skel *dev; int res; diff --git a/fs/afs/internal.h b/fs/afs/internal.h index f38d6a561a84..18b1e2bc2a14 100644 --- a/fs/afs/internal.h +++ b/fs/afs/internal.h @@ -947,7 +947,7 @@ extern int afs_writepage(struct page *, struct writeback_control *); extern int afs_writepages(struct address_space *, struct writeback_control *); extern void afs_pages_written_back(struct afs_vnode *, struct afs_call *); extern ssize_t afs_file_write(struct kiocb *, struct iov_iter *); -extern int afs_flush(struct file *, fl_owner_t); +extern int afs_flush(struct file *, void *); extern int afs_fsync(struct file *, loff_t, loff_t, int); extern int afs_page_mkwrite(struct vm_fault *); extern void afs_prune_wb_keys(struct afs_vnode *); diff --git a/fs/afs/write.c b/fs/afs/write.c index 9370e2feb999..b4ecbe4e46a4 100644 --- a/fs/afs/write.c +++ b/fs/afs/write.c @@ -737,7 +737,7 @@ int afs_fsync(struct file *file, loff_t start, loff_t end, int datasync) * Flush out all outstanding writes on a file opened for writing when it is * closed. */ -int afs_flush(struct file *file, fl_owner_t id) +int afs_flush(struct file *file, void *id) { _enter(""); diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h index 013ba2aed8d9..09de0b6bd6a7 100644 --- a/fs/cifs/cifsfs.h +++ b/fs/cifs/cifsfs.h @@ -108,7 +108,7 @@ extern ssize_t cifs_strict_writev(struct kiocb *iocb, struct iov_iter *from); extern int cifs_lock(struct file *, int, struct file_lock *); extern int cifs_fsync(struct file *, loff_t, loff_t, int); extern int cifs_strict_fsync(struct file *, loff_t, loff_t, int); -extern int cifs_flush(struct file *, fl_owner_t id); +extern int cifs_flush(struct file *, void *id); extern int cifs_file_mmap(struct file * , struct vm_area_struct *); extern int cifs_file_strict_mmap(struct file * , struct vm_area_struct *); extern const struct file_operations cifs_dir_ops; diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 7cee97b93a61..835c155e64ed 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1174,7 +1174,7 @@ cifs_push_mandatory_locks(struct cifsFileInfo *cfile) } static __u32 -hash_lockowner(fl_owner_t owner) +hash_lockowner(void *owner) { return cifs_lock_secret ^ hash32_ptr((const void *)owner); } @@ -2393,7 +2393,7 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync) * As file closes, flush all cached write data for this inode checking * for write behind errors. */ -int cifs_flush(struct file *file, fl_owner_t id) +int cifs_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); int rc = 0; diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c index c74ed3ca3372..78fb58667791 100644 --- a/fs/ecryptfs/file.c +++ b/fs/ecryptfs/file.c @@ -290,7 +290,7 @@ static int ecryptfs_dir_open(struct inode *inode, struct file *file) return 0; } -static int ecryptfs_flush(struct file *file, fl_owner_t td) +static int ecryptfs_flush(struct file *file, void *td) { struct file *lower_file = ecryptfs_file_to_lower(file); diff --git a/fs/exofs/file.c b/fs/exofs/file.c index a94594ea2aa3..86d27d835a11 100644 --- a/fs/exofs/file.c +++ b/fs/exofs/file.c @@ -58,7 +58,7 @@ static int exofs_file_fsync(struct file *filp, loff_t start, loff_t end, return ret; } -static int exofs_flush(struct file *file, fl_owner_t id) +static int exofs_flush(struct file *file, void *id) { int ret = vfs_fsync(file, 0); /* TODO: Flush the OSD target */ diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 672a542e5464..57076d46fd08 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -1557,7 +1557,7 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) return 0; } -static int f2fs_file_flush(struct file *file, fl_owner_t id) +static int f2fs_file_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); diff --git a/fs/fuse/file.c b/fs/fuse/file.c index a201fb0ac64f..746624182249 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -254,8 +254,7 @@ void fuse_release_common(struct file *file, int opcode) if (ff->flock) { struct fuse_release_in *inarg = &req->misc.release.in; inarg->release_flags |= FUSE_RELEASE_FLOCK_UNLOCK; - inarg->lock_owner = fuse_lock_owner_id(ff->fc, - (fl_owner_t) file); + inarg->lock_owner = fuse_lock_owner_id(ff->fc, file); } /* Hold inode until release is finished */ req->misc.release.inode = igrab(file_inode(file)); @@ -307,7 +306,7 @@ EXPORT_SYMBOL_GPL(fuse_sync_release); * Scramble the ID space with XTEA, so that the value of the files_struct * pointer is not exposed to userspace. */ -u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id) +u64 fuse_lock_owner_id(struct fuse_conn *fc, void *id) { u32 *k = fc->scramble_key; u64 v = (unsigned long) id; @@ -390,7 +389,7 @@ static void fuse_sync_writes(struct inode *inode) fuse_release_nowrite(inode); } -static int fuse_flush(struct file *file, fl_owner_t id) +static int fuse_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); struct fuse_conn *fc = get_fuse_conn(inode); @@ -643,7 +642,7 @@ static size_t fuse_async_req_send(struct fuse_conn *fc, struct fuse_req *req, } static size_t fuse_send_read(struct fuse_req *req, struct fuse_io_priv *io, - loff_t pos, size_t count, fl_owner_t owner) + loff_t pos, size_t count, void *owner) { struct file *file = io->iocb->ki_filp; struct fuse_file *ff = file->private_data; @@ -958,7 +957,7 @@ static void fuse_write_fill(struct fuse_req *req, struct fuse_file *ff, } static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io, - loff_t pos, size_t count, fl_owner_t owner) + loff_t pos, size_t count, void *owner) { struct kiocb *iocb = io->iocb; struct file *file = iocb->ki_filp; @@ -1358,7 +1357,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter, io->should_dirty = !write && iter_is_iovec(iter); while (count) { size_t nres; - fl_owner_t owner = current->files; + void *owner = current->files; size_t nbytes = min(count, nmax); err = fuse_get_user_pages(req, iter, &nbytes, write); if (err && !nbytes) diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index c4c093bbf456..e4c2da57c16c 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -900,7 +900,7 @@ int fuse_valid_type(int m); */ int fuse_allow_current_process(struct fuse_conn *fc); -u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id); +u64 fuse_lock_owner_id(struct fuse_conn *fc, void *id); void fuse_update_ctime(struct inode *inode); diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c index a2c0dfc6fdc0..a42dff055260 100644 --- a/fs/lockd/clntproc.c +++ b/fs/lockd/clntproc.c @@ -81,7 +81,7 @@ static inline uint32_t __nlm_alloc_pid(struct nlm_host *host) return res; } -static struct nlm_lockowner *__nlm_find_lockowner(struct nlm_host *host, fl_owner_t owner) +static struct nlm_lockowner *__nlm_find_lockowner(struct nlm_host *host, void *owner) { struct nlm_lockowner *lockowner; list_for_each_entry(lockowner, &host->h_lockowners, list) { @@ -92,7 +92,7 @@ static struct nlm_lockowner *__nlm_find_lockowner(struct nlm_host *host, fl_owne return NULL; } -static struct nlm_lockowner *nlm_find_lockowner(struct nlm_host *host, fl_owner_t owner) +static struct nlm_lockowner *nlm_find_lockowner(struct nlm_host *host, void *owner) { struct nlm_lockowner *res, *new = NULL; diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c index 1bddf70d9656..004777845b61 100644 --- a/fs/lockd/svc4proc.c +++ b/fs/lockd/svc4proc.c @@ -46,7 +46,7 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, /* Set up the missing parts of the file_lock structure */ lock->fl.fl_file = file->f_file; - lock->fl.fl_owner = (fl_owner_t) host; + lock->fl.fl_owner = host; lock->fl.fl_lmops = &nlmsvc_lock_operations; } diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c index ea77c66d3cc3..bbf2329821db 100644 --- a/fs/lockd/svcproc.c +++ b/fs/lockd/svcproc.c @@ -76,7 +76,7 @@ nlmsvc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, /* Set up the missing parts of the file_lock structure */ lock->fl.fl_file = file->f_file; - lock->fl.fl_owner = (fl_owner_t) host; + lock->fl.fl_owner = host; lock->fl.fl_lmops = &nlmsvc_lock_operations; } diff --git a/fs/locks.c b/fs/locks.c index d6ff4beb70ce..63aa52bcdf5a 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -2462,7 +2462,7 @@ int fcntl_setlk64(unsigned int fd, struct file *filp, unsigned int cmd, * from the task's fd array. POSIX locks belonging to this task * are deleted at this time. */ -void locks_remove_posix(struct file *filp, fl_owner_t owner) +void locks_remove_posix(struct file *filp, void *owner) { int error; struct inode *inode = locks_inode(filp); diff --git a/fs/nfs/file.c b/fs/nfs/file.c index 81cca49a8375..922ea233c391 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -136,7 +136,7 @@ EXPORT_SYMBOL_GPL(nfs_file_llseek); * Flush all dirty pages, and check for write errors. */ static int -nfs_file_flush(struct file *file, fl_owner_t id) +nfs_file_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 7d893543cf3b..0b2d6f839bc2 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -927,7 +927,7 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, ctx->mode = f_mode; ctx->flags = 0; ctx->error = 0; - ctx->flock_owner = (fl_owner_t)filp; + ctx->flock_owner = filp; nfs_init_lock_context(&ctx->lock_context); ctx->lock_context.open_context = ctx; INIT_LIST_HEAD(&ctx->list); diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index b374f680830c..5079334cdf37 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -146,7 +146,7 @@ struct nfs4_lock_state { struct nfs_seqid_counter ls_seqid; nfs4_stateid ls_stateid; refcount_t ls_count; - fl_owner_t ls_owner; + void * ls_owner; }; /* bits for nfs4_state->flags */ diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 6b3b372b59b9..7045ff1fb46d 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -107,7 +107,7 @@ nfs4_file_open(struct inode *inode, struct file *filp) * Flush all dirty pages, and check for write errors. */ static int -nfs4_file_flush(struct file *file, fl_owner_t id) +nfs4_file_flush(struct file *file, void *id) { struct inode *inode = file_inode(file); diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 91a4d4eeb235..7938ef7b8b27 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -822,7 +822,7 @@ void nfs4_close_sync(struct nfs4_state *state, fmode_t fmode) */ static struct nfs4_lock_state * __nfs4_find_lock_state(struct nfs4_state *state, - fl_owner_t fl_owner, fl_owner_t fl_owner2) + void *fl_owner, void *fl_owner2) { struct nfs4_lock_state *pos, *ret = NULL; list_for_each_entry(pos, &state->lock_states, ls_locks) { @@ -843,7 +843,7 @@ __nfs4_find_lock_state(struct nfs4_state *state, * exists, return an uninitialized one. * */ -static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, fl_owner_t fl_owner) +static struct nfs4_lock_state *nfs4_alloc_lock_state(struct nfs4_state *state, void *fl_owner) { struct nfs4_lock_state *lsp; struct nfs_server *server = state->owner->so_server; @@ -877,7 +877,7 @@ void nfs4_free_lock_state(struct nfs_server *server, struct nfs4_lock_state *lsp * exists, return an uninitialized one. * */ -static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, fl_owner_t owner) +static struct nfs4_lock_state *nfs4_get_lock_state(struct nfs4_state *state, void *owner) { struct nfs4_lock_state *lsp, *new = NULL; @@ -968,7 +968,7 @@ static int nfs4_copy_lock_stateid(nfs4_stateid *dst, const struct nfs_lock_context *l_ctx) { struct nfs4_lock_state *lsp; - fl_owner_t fl_owner, fl_flock_owner; + void *fl_owner, *fl_flock_owner; int ret = -ENOENT; if (l_ctx == NULL) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 150521c9671b..1bdfefe2e6ec 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1204,7 +1204,7 @@ static void nfs4_free_lock_stateid(struct nfs4_stid *stid) file = find_any_file(stp->st_stid.sc_file); if (file) - filp_close(file, (fl_owner_t)lo); + filp_close(file, lo); nfs4_free_ol_stateid(stid); } @@ -4268,7 +4268,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag) fl->fl_flags = FL_DELEG; fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK; fl->fl_end = OFFSET_MAX; - fl->fl_owner = (fl_owner_t)fp; + fl->fl_owner = fp; fl->fl_pid = current->tgid; return fl; } @@ -5563,8 +5563,8 @@ nfs4_transform_lock_offset(struct file_lock *lock) lock->fl_end = OFFSET_MAX; } -static fl_owner_t -nfsd4_fl_get_owner(fl_owner_t owner) +static void * +nfsd4_fl_get_owner(void *owner) { struct nfs4_lockowner *lo = (struct nfs4_lockowner *)owner; @@ -5573,7 +5573,7 @@ nfsd4_fl_get_owner(fl_owner_t owner) } static void -nfsd4_fl_put_owner(fl_owner_t owner) +nfsd4_fl_put_owner(void *owner) { struct nfs4_lockowner *lo = (struct nfs4_lockowner *)owner; @@ -6008,7 +6008,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, file_lock = &nbl->nbl_lock; file_lock->fl_type = fl_type; - file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(&lock_sop->lo_owner)); + file_lock->fl_owner = lockowner(nfs4_get_stateowner(&lock_sop->lo_owner)); file_lock->fl_pid = current->tgid; file_lock->fl_file = filp; file_lock->fl_flags = fl_flags; @@ -6162,7 +6162,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, lo = find_lockowner_str(cstate->clp, &lockt->lt_owner); if (lo) - file_lock->fl_owner = (fl_owner_t)lo; + file_lock->fl_owner = lo; file_lock->fl_pid = current->tgid; file_lock->fl_flags = FL_POSIX; @@ -6224,7 +6224,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, } file_lock->fl_type = F_UNLCK; - file_lock->fl_owner = (fl_owner_t)lockowner(nfs4_get_stateowner(stp->st_stateowner)); + file_lock->fl_owner = lockowner(nfs4_get_stateowner(stp->st_stateowner)); file_lock->fl_pid = current->tgid; file_lock->fl_file = filp; file_lock->fl_flags = FL_POSIX; @@ -6283,7 +6283,7 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) if (flctx && !list_empty_careful(&flctx->flc_posix)) { spin_lock(&flctx->flc_lock); list_for_each_entry(fl, &flctx->flc_posix, fl_list) { - if (fl->fl_owner == (fl_owner_t)lowner) { + if (fl->fl_owner == lowner) { status = true; break; } diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 63a1ca4b9dee..6b309bd1552d 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -145,7 +145,7 @@ static const struct fsnotify_ops dnotify_fsnotify_ops = { * dnotify_struct. If that was the last dnotify_struct also remove the * fsnotify_mark. */ -void dnotify_flush(struct file *filp, fl_owner_t id) +void dnotify_flush(struct file *filp, void *id) { struct fsnotify_mark *fsn_mark; struct dnotify_mark *dn_mark; @@ -223,7 +223,7 @@ static __u32 convert_arg(unsigned long arg) * that list, or it |= the mask onto an existing dnofiy_struct. */ static int attach_dn(struct dnotify_struct *dn, struct dnotify_mark *dn_mark, - fl_owner_t id, int fd, struct file *filp, __u32 mask) + void *id, int fd, struct file *filp, __u32 mask) { struct dnotify_struct *odn; @@ -259,7 +259,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) struct fsnotify_mark *new_fsn_mark, *fsn_mark; struct dnotify_struct *dn; struct inode *inode; - fl_owner_t id = current->files; + void *id = current->files; struct file *f; int destroy = 0, error = 0; __u32 mask; diff --git a/fs/open.c b/fs/open.c index 7ea118471dce..b04595bca741 100644 --- a/fs/open.c +++ b/fs/open.c @@ -1123,7 +1123,7 @@ SYSCALL_DEFINE2(creat, const char __user *, pathname, umode_t, mode) * "id" is the POSIX thread ID. We use the * files pointer for this.. */ -int filp_close(struct file *filp, fl_owner_t id) +int filp_close(struct file *filp, void *id) { int retval = 0; diff --git a/include/linux/dnotify.h b/include/linux/dnotify.h index 0aad774beaec..4be77f0e8c58 100644 --- a/include/linux/dnotify.h +++ b/include/linux/dnotify.h @@ -14,7 +14,7 @@ struct dnotify_struct { __u32 dn_mask; int dn_fd; struct file * dn_filp; - fl_owner_t dn_owner; + void * dn_owner; }; #ifdef __KERNEL__ @@ -30,12 +30,12 @@ struct dnotify_struct { FS_MOVED_FROM | FS_MOVED_TO) extern int dir_notify_enable; -extern void dnotify_flush(struct file *, fl_owner_t); +extern void dnotify_flush(struct file *, void *); extern int fcntl_dirnotify(int, struct file *, unsigned long); #else -static inline void dnotify_flush(struct file *filp, fl_owner_t id) +static inline void dnotify_flush(struct file *filp, void *id) { } diff --git a/include/linux/fs.h b/include/linux/fs.h index 2a815560fda0..ef9269bf7e69 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -942,9 +942,6 @@ static inline struct file *get_file(struct file *f) */ #define FILE_LOCK_DEFERRED 1 -/* legacy typedef, should eventually be removed */ -typedef void *fl_owner_t; - struct file_lock; struct file_lock_operations { @@ -955,8 +952,8 @@ struct file_lock_operations { struct lock_manager_operations { int (*lm_compare_owner)(struct file_lock *, struct file_lock *); unsigned long (*lm_owner_key)(struct file_lock *); - fl_owner_t (*lm_get_owner)(fl_owner_t); - void (*lm_put_owner)(fl_owner_t); + void *(*lm_get_owner)(void *); + void (*lm_put_owner)(void *); void (*lm_notify)(struct file_lock *); /* unblock callback */ int (*lm_grant)(struct file_lock *, int); bool (*lm_break)(struct file_lock *); @@ -1004,7 +1001,7 @@ struct file_lock { struct list_head fl_list; /* link into file_lock_context */ struct hlist_node fl_link; /* node in global lists */ struct list_head fl_block; /* circular list of blocked processes */ - fl_owner_t fl_owner; + void *fl_owner; unsigned int fl_flags; unsigned char fl_type; unsigned int fl_pid; @@ -1080,7 +1077,7 @@ extern void locks_init_lock(struct file_lock *); extern struct file_lock * locks_alloc_lock(void); extern void locks_copy_lock(struct file_lock *, struct file_lock *); extern void locks_copy_conflock(struct file_lock *, struct file_lock *); -extern void locks_remove_posix(struct file *, fl_owner_t); +extern void locks_remove_posix(struct file *, void *); extern void locks_remove_file(struct file *); extern void locks_release_private(struct file_lock *); extern void posix_test_lock(struct file *, struct file_lock *); @@ -1154,7 +1151,7 @@ static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl) return; } -static inline void locks_remove_posix(struct file *filp, fl_owner_t owner) +static inline void locks_remove_posix(struct file *filp, void *owner) { return; } @@ -1713,7 +1710,7 @@ struct file_operations { int (*mmap) (struct file *, struct vm_area_struct *); unsigned long mmap_supported_flags; int (*open) (struct inode *, struct file *); - int (*flush) (struct file *, fl_owner_t id); + int (*flush) (struct file *, void *id); int (*release) (struct inode *, struct file *); int (*fsync) (struct file *, loff_t, loff_t, int datasync); int (*fasync) (int, struct file *, int); @@ -2397,7 +2394,7 @@ extern struct file *filp_open(const char *, int, umode_t); extern struct file *file_open_root(struct dentry *, struct vfsmount *, const char *, int, umode_t); extern struct file * dentry_open(const struct path *, int, const struct cred *); -extern int filp_close(struct file *, fl_owner_t id); +extern int filp_close(struct file *, void *id); extern struct filename *getname_flags(const char __user *, int, int *); extern struct filename *getname(const char __user *); diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h index 4fd95dbeb52f..484156a8f11a 100644 --- a/include/linux/lockd/lockd.h +++ b/include/linux/lockd/lockd.h @@ -119,14 +119,14 @@ static inline struct sockaddr *nlm_srcaddr(const struct nlm_host *host) } /* - * Map an fl_owner_t into a unique 32-bit "pid" + * Map a lock owner into a unique 32-bit "pid" */ struct nlm_lockowner { struct list_head list; refcount_t count; struct nlm_host *host; - fl_owner_t owner; + void *owner; uint32_t pid; }; diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 38187c68063d..889d3494dbbb 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -60,14 +60,14 @@ struct nfs_lock_context { refcount_t count; struct list_head list; struct nfs_open_context *open_context; - fl_owner_t lockowner; + void *lockowner; atomic_t io_count; }; struct nfs4_state; struct nfs_open_context { struct nfs_lock_context lock_context; - fl_owner_t flock_owner; + void *flock_owner; struct dentry *dentry; struct rpc_cred *cred; struct nfs4_state *state; diff --git a/include/trace/events/filelock.h b/include/trace/events/filelock.h index d1faf3597b9d..345fdb312cab 100644 --- a/include/trace/events/filelock.h +++ b/include/trace/events/filelock.h @@ -69,7 +69,7 @@ DECLARE_EVENT_CLASS(filelock_lock, __field(unsigned long, i_ino) __field(dev_t, s_dev) __field(struct file_lock *, fl_next) - __field(fl_owner_t, fl_owner) + __field(void *, fl_owner) __field(unsigned int, fl_pid) __field(unsigned int, fl_flags) __field(unsigned char, fl_type) @@ -123,7 +123,7 @@ DECLARE_EVENT_CLASS(filelock_lease, __field(unsigned long, i_ino) __field(dev_t, s_dev) __field(struct file_lock *, fl_next) - __field(fl_owner_t, fl_owner) + __field(void *, fl_owner) __field(unsigned int, fl_flags) __field(unsigned char, fl_type) __field(unsigned long, fl_break_time) @@ -176,7 +176,7 @@ TRACE_EVENT(generic_add_lease, __field(int, dcount) __field(int, icount) __field(dev_t, s_dev) - __field(fl_owner_t, fl_owner) + __field(void *, fl_owner) __field(unsigned int, fl_flags) __field(unsigned char, fl_type) ), diff --git a/ipc/mqueue.c b/ipc/mqueue.c index d7f309f74dec..197cb72af8de 100644 --- a/ipc/mqueue.c +++ b/ipc/mqueue.c @@ -557,7 +557,7 @@ static ssize_t mqueue_read_file(struct file *filp, char __user *u_data, return ret; } -static int mqueue_flush_file(struct file *filp, fl_owner_t id) +static int mqueue_flush_file(struct file *filp, void *id) { struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp)); -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 02/10] nfsd: simplify put of fi_deleg_file 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields 2018-03-19 14:36 ` [PATCH 01/10] vfs: remove unnecessary fl_owner_t typedef J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 03/10] nfsd: simplify nfs4_put_deleg_lease calls J. Bruce Fields ` (9 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> fi_delegees is basically just a reference count on users of fi_deleg_file, which is cleared when fi_delegees goes to zero. The fi_deleg_file check here is redundant. Also add an assertion to make sure we don't have unbalanced puts. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 1bdfefe2e6ec..1a0b38fddde4 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -849,9 +849,12 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp) { struct file *filp = NULL; + WARN_ON_ONCE(!fp->fi_delegees); + spin_lock(&fp->fi_lock); - if (fp->fi_deleg_file && --fp->fi_delegees == 0) + if (--fp->fi_delegees == 0) { swap(filp, fp->fi_deleg_file); + } spin_unlock(&fp->fi_lock); if (filp) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 03/10] nfsd: simplify nfs4_put_deleg_lease calls 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields 2018-03-19 14:36 ` [PATCH 01/10] vfs: remove unnecessary fl_owner_t typedef J. Bruce Fields 2018-03-19 14:36 ` [PATCH 02/10] nfsd: simplify put of fi_deleg_file J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 04/10] nfsd4: set fl_owner to delegation, not file pointer J. Bruce Fields ` (8 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> Every single caller gets the file out of the delegation, so let's do that once in nfs4_put_deleg_lease. Plus we'll need it there for other reasons. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 1a0b38fddde4..51f633c85776 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -845,9 +845,10 @@ nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid) spin_unlock(&stid->sc_lock); } -static void nfs4_put_deleg_lease(struct nfs4_file *fp) +static void nfs4_put_deleg_lease(struct nfs4_delegation *dp) { struct file *filp = NULL; + struct nfs4_file *fp = dp->dl_stid.sc_file; WARN_ON_ONCE(!fp->fi_delegees); @@ -962,7 +963,7 @@ static void destroy_delegation(struct nfs4_delegation *dp) spin_unlock(&state_lock); if (unhashed) { put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp->dl_stid.sc_file); + nfs4_put_deleg_lease(dp); nfs4_put_stid(&dp->dl_stid); } } @@ -974,7 +975,7 @@ static void revoke_delegation(struct nfs4_delegation *dp) WARN_ON(!list_empty(&dp->dl_recall_lru)); put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp->dl_stid.sc_file); + nfs4_put_deleg_lease(dp); if (clp->cl_minorversion == 0) nfs4_put_stid(&dp->dl_stid); @@ -1885,7 +1886,7 @@ __destroy_client(struct nfs4_client *clp) dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru); list_del_init(&dp->dl_recall_lru); put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp->dl_stid.sc_file); + nfs4_put_deleg_lease(dp); nfs4_put_stid(&dp->dl_stid); } while (!list_empty(&clp->cl_revoked)) { @@ -7226,7 +7227,7 @@ nfs4_state_shutdown_net(struct net *net) dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru); list_del_init(&dp->dl_recall_lru); put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp->dl_stid.sc_file); + nfs4_put_deleg_lease(dp); nfs4_put_stid(&dp->dl_stid); } -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 04/10] nfsd4: set fl_owner to delegation, not file pointer 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (2 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 03/10] nfsd: simplify nfs4_put_deleg_lease calls J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 05/10] nfsd4: dp->dl_stid.sc_file doesn't need locking J. Bruce Fields ` (7 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> For now this makes no difference, as for files having delegations, there's a one-to-one relationship between an nfs4_file and its nfs4_delegation. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 51f633c85776..0466312a2a4e 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -859,7 +859,7 @@ static void nfs4_put_deleg_lease(struct nfs4_delegation *dp) spin_unlock(&fp->fi_lock); if (filp) { - vfs_setlease(filp, F_UNLCK, NULL, (void **)&fp); + vfs_setlease(filp, F_UNLCK, NULL, (void **)&dp); fput(filp); } } @@ -3909,13 +3909,9 @@ static bool nfsd_break_deleg_cb(struct file_lock *fl) { bool ret = false; - struct nfs4_file *fp = (struct nfs4_file *)fl->fl_owner; - struct nfs4_delegation *dp; + struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner; + struct nfs4_file *fp = dp->dl_stid.sc_file; - if (!fp) { - WARN(1, "(%p)->fl_owner NULL\n", fl); - return ret; - } if (fp->fi_had_conflict) { WARN(1, "duplicate break on %p\n", fp); return ret; @@ -4261,7 +4257,8 @@ static bool nfsd4_cb_channel_good(struct nfs4_client *clp) return clp->cl_minorversion && clp->cl_cb_state == NFSD4_CB_UNKNOWN; } -static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag) +static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, + int flag) { struct file_lock *fl; @@ -4272,7 +4269,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag) fl->fl_flags = FL_DELEG; fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK; fl->fl_end = OFFSET_MAX; - fl->fl_owner = fp; + fl->fl_owner = dp; fl->fl_pid = current->tgid; return fl; } @@ -4296,7 +4293,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp) struct file *filp; int status = 0; - fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ); + fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); if (!fl) return -ENOMEM; filp = find_readable_file(fp); -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 05/10] nfsd4: dp->dl_stid.sc_file doesn't need locking 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (3 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 04/10] nfsd4: set fl_owner to delegation, not file pointer J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 06/10] nfsd: make nfs4_get_existing_delegation less confusing J. Bruce Fields ` (6 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> The delegation isn't visible to anyone yet. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0466312a2a4e..3fc0682ca799 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4363,9 +4363,10 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, return ERR_PTR(-ENOMEM); get_nfs4_file(fp); + dp->dl_stid.sc_file = fp; + spin_lock(&state_lock); spin_lock(&fp->fi_lock); - dp->dl_stid.sc_file = fp; if (!fp->fi_deleg_file) { spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 06/10] nfsd: make nfs4_get_existing_delegation less confusing 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (4 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 05/10] nfsd4: dp->dl_stid.sc_file doesn't need locking J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 07/10] nfsd: factor out common delegation-destruction code J. Bruce Fields ` (5 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> This doesn't "get" anything. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 3fc0682ca799..b49000797c6f 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -870,20 +870,16 @@ void nfs4_unhash_stid(struct nfs4_stid *s) } /** - * nfs4_get_existing_delegation - Discover if this delegation already exists + * nfs4_delegation_exists - Discover if this delegation already exists * @clp: a pointer to the nfs4_client we're granting a delegation to * @fp: a pointer to the nfs4_file we're granting a delegation on * * Return: - * On success: NULL if an existing delegation was not found. - * - * On error: -EAGAIN if one was previously granted to this nfs4_client - * for this nfs4_file. - * + * On success: true iff an existing delegation is found */ -static int -nfs4_get_existing_delegation(struct nfs4_client *clp, struct nfs4_file *fp) +static bool +nfs4_delegation_exists(struct nfs4_client *clp, struct nfs4_file *fp) { struct nfs4_delegation *searchdp = NULL; struct nfs4_client *searchclp = NULL; @@ -916,15 +912,13 @@ nfs4_get_existing_delegation(struct nfs4_client *clp, struct nfs4_file *fp) static int hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) { - int status; struct nfs4_client *clp = dp->dl_stid.sc_client; lockdep_assert_held(&state_lock); lockdep_assert_held(&fp->fi_lock); - status = nfs4_get_existing_delegation(clp, fp); - if (status) - return status; + if (nfs4_delegation_exists(clp, fp)) + return -EAGAIN; ++fp->fi_delegees; refcount_inc(&dp->dl_stid.sc_count); dp->dl_stid.sc_type = NFS4_DELEG_STID; @@ -4343,7 +4337,7 @@ static struct nfs4_delegation * nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) { - int status; + int status = 0; struct nfs4_delegation *dp; if (fp->fi_had_conflict) @@ -4351,7 +4345,8 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, spin_lock(&state_lock); spin_lock(&fp->fi_lock); - status = nfs4_get_existing_delegation(clp, fp); + if (nfs4_delegation_exists(clp, fp)) + status = -EAGAIN; spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 07/10] nfsd: factor out common delegation-destruction code 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (5 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 06/10] nfsd: make nfs4_get_existing_delegation less confusing J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 08/10] nfsd: move sc_file assignment into alloc_init_deleg J. Bruce Fields ` (4 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> Pull some duplicated code into a common helper. This changes the order in destroy_delegation a little, but it looks to me like that shouldn't matter. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index b49000797c6f..268d095477e8 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -864,6 +864,13 @@ static void nfs4_put_deleg_lease(struct nfs4_delegation *dp) } } +static void destroy_unhashed_deleg(struct nfs4_delegation *dp) +{ + put_clnt_odstate(dp->dl_clnt_odstate); + nfs4_put_deleg_lease(dp); + nfs4_put_stid(&dp->dl_stid); +} + void nfs4_unhash_stid(struct nfs4_stid *s) { s->sc_type = 0; @@ -955,11 +962,8 @@ static void destroy_delegation(struct nfs4_delegation *dp) spin_lock(&state_lock); unhashed = unhash_delegation_locked(dp); spin_unlock(&state_lock); - if (unhashed) { - put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp); - nfs4_put_stid(&dp->dl_stid); - } + if (unhashed) + destroy_unhashed_deleg(dp); } static void revoke_delegation(struct nfs4_delegation *dp) @@ -968,17 +972,14 @@ static void revoke_delegation(struct nfs4_delegation *dp) WARN_ON(!list_empty(&dp->dl_recall_lru)); - put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp); - - if (clp->cl_minorversion == 0) - nfs4_put_stid(&dp->dl_stid); - else { + if (clp->cl_minorversion) { dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID; + refcount_inc(&dp->dl_stid.sc_count); spin_lock(&clp->cl_lock); list_add(&dp->dl_recall_lru, &clp->cl_revoked); spin_unlock(&clp->cl_lock); } + destroy_unhashed_deleg(dp); } /* @@ -1879,9 +1880,7 @@ __destroy_client(struct nfs4_client *clp) while (!list_empty(&reaplist)) { dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru); list_del_init(&dp->dl_recall_lru); - put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp); - nfs4_put_stid(&dp->dl_stid); + destroy_unhashed_deleg(dp); } while (!list_empty(&clp->cl_revoked)) { dp = list_entry(clp->cl_revoked.next, struct nfs4_delegation, dl_recall_lru); @@ -7219,9 +7218,7 @@ nfs4_state_shutdown_net(struct net *net) list_for_each_safe(pos, next, &reaplist) { dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru); list_del_init(&dp->dl_recall_lru); - put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp); - nfs4_put_stid(&dp->dl_stid); + destroy_unhashed_deleg(dp); } BUG_ON(!list_empty(&reaplist)); -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 08/10] nfsd: move sc_file assignment into alloc_init_deleg 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (6 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 07/10] nfsd: factor out common delegation-destruction code J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 09/10] nfsd: create a separate lease for each delegation J. Bruce Fields ` (3 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> Take an easy chance to simplify the caller a little. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 268d095477e8..26880d0122c5 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -777,7 +777,8 @@ static void block_delegations(struct knfsd_fh *fh) } static struct nfs4_delegation * -alloc_init_deleg(struct nfs4_client *clp, struct svc_fh *current_fh, +alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp, + struct svc_fh *current_fh, struct nfs4_clnt_odstate *odstate) { struct nfs4_delegation *dp; @@ -808,6 +809,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct svc_fh *current_fh, dp->dl_retries = 1; nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client, &nfsd4_cb_recall_ops, NFSPROC4_CLNT_CB_RECALL); + get_nfs4_file(fp); + dp->dl_stid.sc_file = fp; return dp; out_dec: atomic_long_dec(&num_delegations); @@ -4352,13 +4355,10 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, if (status) return ERR_PTR(status); - dp = alloc_init_deleg(clp, fh, odstate); + dp = alloc_init_deleg(clp, fp, fh, odstate); if (!dp) return ERR_PTR(-ENOMEM); - get_nfs4_file(fp); - dp->dl_stid.sc_file = fp; - spin_lock(&state_lock); spin_lock(&fp->fi_lock); if (!fp->fi_deleg_file) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 09/10] nfsd: create a separate lease for each delegation 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (7 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 08/10] nfsd: move sc_file assignment into alloc_init_deleg J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-19 14:36 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations J. Bruce Fields ` (2 subsequent siblings) 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> Currently we only take one vfs-level delegation (lease) for each file, no matter how many clients hold delegations on that file. Let's instead keep a one-to-one mapping between NFSv4 delegations and VFS delegations. This turns out to be simpler. There is still a many-to-one mapping of NFS opens to NFS files, and the delegations on one file are all associated with one struct file. The VFS can still distinguish between these delegations since we're setting fl_owner to the struct nfs4_delegation now, not to the shared file. I'm replacing at least one complicated function wholesale, which I don't like to do, but I haven't figured out how to do this more incrementally. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 173 ++++++++++++++++++++-------------------------------- 1 file changed, 65 insertions(+), 108 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 26880d0122c5..ca8a5644d9ff 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -848,29 +848,34 @@ nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid) spin_unlock(&stid->sc_lock); } -static void nfs4_put_deleg_lease(struct nfs4_delegation *dp) +static void put_deleg_file(struct nfs4_file *fp) { struct file *filp = NULL; - struct nfs4_file *fp = dp->dl_stid.sc_file; - - WARN_ON_ONCE(!fp->fi_delegees); spin_lock(&fp->fi_lock); - if (--fp->fi_delegees == 0) { + if (--fp->fi_delegees == 0) swap(filp, fp->fi_deleg_file); - } spin_unlock(&fp->fi_lock); - if (filp) { - vfs_setlease(filp, F_UNLCK, NULL, (void **)&dp); + if (filp) fput(filp); - } +} + +static void nfs4_unlock_deleg_lease(struct nfs4_delegation *dp) +{ + struct nfs4_file *fp = dp->dl_stid.sc_file; + struct file *filp = fp->fi_deleg_file; + + WARN_ON_ONCE(!fp->fi_delegees); + + vfs_setlease(filp, F_UNLCK, NULL, (void **)&dp); + put_deleg_file(fp); } static void destroy_unhashed_deleg(struct nfs4_delegation *dp) { put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_deleg_lease(dp); + nfs4_unlock_deleg_lease(dp); nfs4_put_stid(&dp->dl_stid); } @@ -929,7 +934,6 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) if (nfs4_delegation_exists(clp, fp)) return -EAGAIN; - ++fp->fi_delegees; refcount_inc(&dp->dl_stid.sc_count); dp->dl_stid.sc_type = NFS4_DELEG_STID; list_add(&dp->dl_perfile, &fp->fi_delegations); @@ -3908,10 +3912,6 @@ nfsd_break_deleg_cb(struct file_lock *fl) struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner; struct nfs4_file *fp = dp->dl_stid.sc_file; - if (fp->fi_had_conflict) { - WARN(1, "duplicate break on %p\n", fp); - return ret; - } /* * We don't want the locks code to timeout the lease for us; * we'll remove it ourself if a delegation isn't returned @@ -3921,15 +3921,7 @@ nfsd_break_deleg_cb(struct file_lock *fl) spin_lock(&fp->fi_lock); fp->fi_had_conflict = true; - /* - * If there are no delegations on the list, then return true - * so that the lease code will go ahead and delete it. - */ - if (list_empty(&fp->fi_delegations)) - ret = true; - else - list_for_each_entry(dp, &fp->fi_delegations, dl_perfile) - nfsd_break_one_deleg(dp); + nfsd_break_one_deleg(dp); spin_unlock(&fp->fi_lock); return ret; } @@ -4267,121 +4259,86 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, fl->fl_end = OFFSET_MAX; fl->fl_owner = dp; fl->fl_pid = current->tgid; + fl->fl_file = dp->dl_stid.sc_file->fi_deleg_file; return fl; } -/** - * nfs4_setlease - Obtain a delegation by requesting lease from vfs layer - * @dp: a pointer to the nfs4_delegation we're adding. - * - * Return: - * On success: Return code will be 0 on success. - * - * On error: -EAGAIN if there was an existing delegation. - * nonzero if there is an error in other cases. - * - */ - -static int nfs4_setlease(struct nfs4_delegation *dp) -{ - struct nfs4_file *fp = dp->dl_stid.sc_file; - struct file_lock *fl; - struct file *filp; - int status = 0; - - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); - if (!fl) - return -ENOMEM; - filp = find_readable_file(fp); - if (!filp) { - /* We should always have a readable file here */ - WARN_ON_ONCE(1); - locks_free_lock(fl); - return -EBADF; - } - fl->fl_file = filp; - status = vfs_setlease(filp, fl->fl_type, &fl, NULL); - if (fl) - locks_free_lock(fl); - if (status) - goto out_fput; - spin_lock(&state_lock); - spin_lock(&fp->fi_lock); - /* Did the lease get broken before we took the lock? */ - status = -EAGAIN; - if (fp->fi_had_conflict) - goto out_unlock; - /* Race breaker */ - if (fp->fi_deleg_file) { - status = hash_delegation_locked(dp, fp); - goto out_unlock; - } - fp->fi_deleg_file = filp; - fp->fi_delegees = 0; - status = hash_delegation_locked(dp, fp); - spin_unlock(&fp->fi_lock); - spin_unlock(&state_lock); - if (status) { - /* Should never happen, this is a new fi_deleg_file */ - WARN_ON_ONCE(1); - goto out_fput; - } - return 0; -out_unlock: - spin_unlock(&fp->fi_lock); - spin_unlock(&state_lock); -out_fput: - fput(filp); - return status; -} - static struct nfs4_delegation * nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh, struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate) { int status = 0; struct nfs4_delegation *dp; + struct file *filp; + struct file_lock *fl; + /* + * The fi_had_conflict and nfs_get_existing_delegation checks + * here are just optimizations; we'll need to recheck them at + * the end: + */ if (fp->fi_had_conflict) return ERR_PTR(-EAGAIN); + filp = find_readable_file(fp); + if (!filp) { + /* We should always have a readable file here */ + WARN_ON_ONCE(1); + return ERR_PTR(-EBADF); + } spin_lock(&state_lock); spin_lock(&fp->fi_lock); if (nfs4_delegation_exists(clp, fp)) status = -EAGAIN; + else if (!fp->fi_deleg_file) { + fp->fi_deleg_file = filp; + /* increment early to prevent fi_deleg_file from being + * cleared */ + fp->fi_delegees = 1; + filp = NULL; + } else + fp->fi_delegees++; spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); - + if (filp) + fput(filp); if (status) return ERR_PTR(status); + status = -ENOMEM; dp = alloc_init_deleg(clp, fp, fh, odstate); if (!dp) - return ERR_PTR(-ENOMEM); + goto out_delegees; + + fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ); + if (!fl) + goto out_stid; + + status = vfs_setlease(fp->fi_deleg_file, fl->fl_type, &fl, NULL); + if (fl) + locks_free_lock(fl); + if (status) + goto out_clnt_odstate; spin_lock(&state_lock); spin_lock(&fp->fi_lock); - if (!fp->fi_deleg_file) { - spin_unlock(&fp->fi_lock); - spin_unlock(&state_lock); - status = nfs4_setlease(dp); - goto out; - } - if (fp->fi_had_conflict) { + if (fp->fi_had_conflict) status = -EAGAIN; - goto out_unlock; - } - status = hash_delegation_locked(dp, fp); -out_unlock: + else + status = hash_delegation_locked(dp, fp); spin_unlock(&fp->fi_lock); spin_unlock(&state_lock); -out: - if (status) { - put_clnt_odstate(dp->dl_clnt_odstate); - nfs4_put_stid(&dp->dl_stid); - return ERR_PTR(status); - } + + if (status) + destroy_unhashed_deleg(dp); return dp; +out_clnt_odstate: + put_clnt_odstate(dp->dl_clnt_odstate); +out_stid: + nfs4_put_stid(&dp->dl_stid); +out_delegees: + put_deleg_file(fp); + return ERR_PTR(status); } static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status) -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (8 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 09/10] nfsd: create a separate lease for each delegation J. Bruce Fields @ 2018-03-19 14:36 ` J. Bruce Fields 2018-03-20 13:10 ` [PATCH 00/10] Eliminate delegation self-conflicts v2 Jeff Layton 2018-03-20 13:35 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations David Howells 11 siblings, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-19 14:36 UTC (permalink / raw) To: linux-nfs, linux-fsdevel; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> We currently revoke read delegations on any write open or any operation that modifies file data or metadata (including rename, link, and unlink). But if the delegation in question is the only read delegation and is held by the client performing the operation, that's not really necessary. It's not always possible to prevent this in the NFSv4.0 case, because there's not always a way to determine which client an NFSv4.0 delegation came from. (In theory we could try to guess this from the transport layer, e.g., by assuming all traffic on a given TCP connection comes from the same client. But that's not really correct.) In the NFSv4.1 case the session layer always tells us the client. This patch should remove such self-conflicts in all cases where we can reliably determine the client from the compound. To do that we need to track "who" is performing a given (possibly lease-breaking) file operation. There are a lot of those. So it seems simpler to pass this in struct task; this patch uses a new field in struct cred. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- Documentation/filesystems/Locking | 2 ++ fs/locks.c | 4 ++++ fs/nfsd/auth.c | 2 ++ fs/nfsd/nfs4proc.c | 1 + fs/nfsd/nfs4state.c | 8 ++++++++ fs/nfsd/nfssvc.c | 1 + include/linux/cred.h | 3 +++ include/linux/fs.h | 1 + include/linux/sunrpc/svc.h | 1 + 9 files changed, 23 insertions(+) diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking index 75d2d57e2c44..739c55825330 100644 --- a/Documentation/filesystems/Locking +++ b/Documentation/filesystems/Locking @@ -366,6 +366,7 @@ prototypes: int (*lm_grant)(struct file_lock *, struct file_lock *, int); void (*lm_break)(struct file_lock *); /* break_lease callback */ int (*lm_change)(struct file_lock **, int); + bool (*lm_breaker_owns_lease)(void *, struct file_lock *); locking rules: @@ -376,6 +377,7 @@ lm_notify: yes yes no lm_grant: no no no lm_break: yes no no lm_change yes no no +lm_breaker_owns_lease: no no no [1]: ->lm_compare_owner and ->lm_owner_key are generally called with *an* inode->i_lock held. It may not be the i_lock of the inode diff --git a/fs/locks.c b/fs/locks.c index 63aa52bcdf5a..22ed02b20559 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1414,6 +1414,9 @@ static void time_out_leases(struct inode *inode, struct list_head *dispose) static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker) { + if (lease->fl_lmops->lm_breaker_owns_lease && breaker->fl_owner && + lease->fl_lmops->lm_breaker_owns_lease(breaker->fl_owner, lease)) + return false; if ((breaker->fl_flags & FL_LAYOUT) != (lease->fl_flags & FL_LAYOUT)) return false; if ((breaker->fl_flags & FL_DELEG) && (lease->fl_flags & FL_LEASE)) @@ -1462,6 +1465,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type) if (IS_ERR(new_fl)) return PTR_ERR(new_fl); new_fl->fl_flags = type; + new_fl->fl_owner = current_cred()->lease_breaker; /* typically we will check that ctx is non-NULL before calling */ ctx = smp_load_acquire(&inode->i_flctx); diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c index fdf2aad73470..d2562ae21b8e 100644 --- a/fs/nfsd/auth.c +++ b/fs/nfsd/auth.c @@ -81,6 +81,8 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp) else new->cap_effective = cap_raise_nfsd_set(new->cap_effective, new->cap_permitted); + if (rqstp->rq_lease_breaker) + new->lease_breaker = *rqstp->rq_lease_breaker; validate_process_creds(); put_cred(override_creds(new)); put_cred(new); diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index a0bed2b2004d..e1341dbaf657 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -1713,6 +1713,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp) op->status = status; goto encode_op; } + rqstp->rq_lease_breaker = (void **)&cstate->clp; while (!status && resp->opcnt < args->opcnt) { op = &args->ops[resp->opcnt++]; diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index ca8a5644d9ff..4036544b5b66 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3926,6 +3926,13 @@ nfsd_break_deleg_cb(struct file_lock *fl) return ret; } +static bool nfsd_breaker_owns_lease(void *breaker, struct file_lock *fl) +{ + struct nfs4_delegation *dl = fl->fl_owner; + + return dl->dl_stid.sc_client == breaker; +} + static int nfsd_change_deleg_cb(struct file_lock *onlist, int arg, struct list_head *dispose) @@ -3937,6 +3944,7 @@ nfsd_change_deleg_cb(struct file_lock *onlist, int arg, } static const struct lock_manager_operations nfsd_lease_mng_ops = { + .lm_breaker_owns_lease = nfsd_breaker_owns_lease, .lm_break = nfsd_break_deleg_cb, .lm_change = nfsd_change_deleg_cb, }; diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index 89cb484f1cfb..c4e86a0a8dd3 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -803,6 +803,7 @@ nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp) *statp = rpc_garbage_args; return 1; } + rqstp->rq_lease_breaker = NULL; /* * Give the xdr decoder a chance to change this if it wants * (necessary in the NFSv4.0 compound case) diff --git a/include/linux/cred.h b/include/linux/cred.h index 631286535d0f..d567c27eebae 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -139,6 +139,9 @@ struct cred { struct key *thread_keyring; /* keyring private to this thread */ struct key *request_key_auth; /* assumed request_key authority */ #endif +#ifdef CONFIG_FILE_LOCKING + void *lease_breaker; /* identify NFS client breaking a delegation */ +#endif #ifdef CONFIG_SECURITY void *security; /* subjective LSM security */ #endif diff --git a/include/linux/fs.h b/include/linux/fs.h index ef9269bf7e69..43e1d2f47cb3 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -959,6 +959,7 @@ struct lock_manager_operations { bool (*lm_break)(struct file_lock *); int (*lm_change)(struct file_lock *, int, struct list_head *); void (*lm_setup)(struct file_lock *, void **); + bool (*lm_breaker_owns_lease)(void *, struct file_lock *); }; struct lock_manager { diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index 786ae2255f05..f2bbfee1662a 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -293,6 +293,7 @@ struct svc_rqst { struct svc_cacherep * rq_cacherep; /* cache info */ struct task_struct *rq_task; /* service thread */ spinlock_t rq_lock; /* per-request lock */ + void ** rq_lease_breaker; /* The v4 client breaking a lease */ }; #define SVC_NET(svc_rqst) (svc_rqst->rq_xprt->xpt_net) -- 2.14.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 00/10] Eliminate delegation self-conflicts v2 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (9 preceding siblings ...) 2018-03-19 14:36 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations J. Bruce Fields @ 2018-03-20 13:10 ` Jeff Layton 2018-03-20 13:35 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations David Howells 11 siblings, 0 replies; 19+ messages in thread From: Jeff Layton @ 2018-03-20 13:10 UTC (permalink / raw) To: J. Bruce Fields, linux-nfs, linux-fsdevel On Mon, 2018-03-19 at 10:36 -0400, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > This is my second attempt to fix the NFS server so we don't > unnecessarily recall delegations when the operation breaking the > delegation comes from the same client that holds the delegation. > > To do that we need some way to pass the identity of the breaker down > through the VFS. In my first attempt I tried passing that explicitly, > but that touches a lot of code. So instead I'm experimenting with > adding a field to the cred struct. > > Testing has confirmed that this works. (And that the pynfs tests are > broken--they *require* the server to break delegations on operations > coming from the client, even though that's the less desireable behavior. > I'm fixing that...). > > J. Bruce Fields (10): > vfs: remove unnecessary fl_owner_t typedef > nfsd: simplify put of fi_deleg_file > nfsd: simplify nfs4_put_deleg_lease calls > nfsd4: set fl_owner to delegation, not file pointer > nfsd4: dp->dl_stid.sc_file doesn't need locking > nfsd: make nfs4_get_existing_delegation less confusing > nfsd: factor out common delegation-destruction code > nfsd: move sc_file assignment into alloc_init_deleg > nfsd: create a separate lease for each delegation > nfsd: clients don't need to break their own delegations > > Documentation/filesystems/Locking | 2 + > Documentation/filesystems/vfs.txt | 2 +- > arch/ia64/kernel/perfmon.c | 2 +- > arch/powerpc/platforms/cell/spufs/file.c | 2 +- > arch/tile/kernel/hardwall.c | 2 +- > drivers/android/binder.c | 2 +- > drivers/char/ps3flash.c | 2 +- > drivers/char/xillybus/xillybus_core.c | 2 +- > drivers/firmware/efi/capsule-loader.c | 2 +- > drivers/input/evdev.c | 2 +- > drivers/misc/mic/scif/scif_fd.c | 2 +- > drivers/scsi/osst.c | 2 +- > drivers/scsi/st.c | 2 +- > drivers/staging/lustre/lustre/llite/file.c | 2 +- > drivers/usb/class/cdc-wdm.c | 2 +- > drivers/usb/usb-skeleton.c | 2 +- > fs/afs/internal.h | 2 +- > fs/afs/write.c | 2 +- > fs/cifs/cifsfs.h | 2 +- > fs/cifs/file.c | 4 +- > fs/ecryptfs/file.c | 2 +- > fs/exofs/file.c | 2 +- > fs/f2fs/file.c | 2 +- > fs/fuse/file.c | 13 +- > fs/fuse/fuse_i.h | 2 +- > fs/lockd/clntproc.c | 4 +- > fs/lockd/svc4proc.c | 2 +- > fs/lockd/svcproc.c | 2 +- > fs/locks.c | 6 +- > fs/nfs/file.c | 2 +- > fs/nfs/inode.c | 2 +- > fs/nfs/nfs4_fs.h | 2 +- > fs/nfs/nfs4file.c | 2 +- > fs/nfs/nfs4state.c | 8 +- > fs/nfsd/auth.c | 2 + > fs/nfsd/nfs4proc.c | 1 + > fs/nfsd/nfs4state.c | 267 ++++++++++++----------------- > fs/nfsd/nfssvc.c | 1 + > fs/notify/dnotify/dnotify.c | 6 +- > fs/open.c | 2 +- > include/linux/cred.h | 3 + > include/linux/dnotify.h | 6 +- > include/linux/fs.h | 18 +- > include/linux/lockd/lockd.h | 4 +- > include/linux/nfs_fs.h | 4 +- > include/linux/sunrpc/svc.h | 1 + > include/trace/events/filelock.h | 6 +- > ipc/mqueue.c | 2 +- > 48 files changed, 193 insertions(+), 223 deletions(-) > Nice work. The whole first part of the series looks like a nice set of cleanups (sans the first patch, as there seems to be support to keep fl_owner_t typedef around). Giving each delegation its own lease makes a lot more sense, IMO. On patches 2-9, you can add: Reviewed-by: Jeff Layton <jlayton@kernel.org> ...and maybe get those into -next soon? The delegation code has been a source of subtle bugs in the past, so we really want this to get a lot of testing. With patch 10, I have some concern about growing a common structure like struct cred with such a special-purpose field. An alternative might be to utilize the keyrings there -- maybe stash a special sort of key on one of the keyrings with this pointer? We'd need to take care to prevent userland from doing this, but that seems like a solvable problem. Cheers, -- Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields ` (10 preceding siblings ...) 2018-03-20 13:10 ` [PATCH 00/10] Eliminate delegation self-conflicts v2 Jeff Layton @ 2018-03-20 13:35 ` David Howells 2018-03-20 13:46 ` Trond Myklebust 2018-03-20 14:52 ` J. Bruce Fields 11 siblings, 2 replies; 19+ messages in thread From: David Howells @ 2018-03-20 13:35 UTC (permalink / raw) To: J. Bruce Fields; +Cc: dhowells, linux-nfs, linux-fsdevel J. Bruce Fields <bfields@redhat.com> wrote: > @@ -139,6 +139,9 @@ struct cred { > struct key *thread_keyring; /* keyring private to this thread */ > struct key *request_key_auth; /* assumed request_key authority */ > #endif > +#ifdef CONFIG_FILE_LOCKING > + void *lease_breaker; /* identify NFS client breaking a delegation */ > +#endif > #ifdef CONFIG_SECURITY > void *security; /* subjective LSM security */ > #endif Sorry, but ewww. Two reasons for that comment: (1) The cred struct may get retained long past where you expect if it gets attached to another process or a file descriptor. (2) The ->lease_breaker pointer needs lifetime management in cred.c. It will potentially get copied around and may need cleaning up. Can you stick your breaker identity in a key struct as Jeff suggested? David ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 13:35 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations David Howells @ 2018-03-20 13:46 ` Trond Myklebust 2018-03-20 14:49 ` J. Bruce Fields 2018-03-20 14:52 ` J. Bruce Fields 1 sibling, 1 reply; 19+ messages in thread From: Trond Myklebust @ 2018-03-20 13:46 UTC (permalink / raw) To: bfields, dhowells; +Cc: linux-nfs, linux-fsdevel On Tue, 2018-03-20 at 13:35 +0000, David Howells wrote: > J. Bruce Fields <bfields@redhat.com> wrote: > > > @@ -139,6 +139,9 @@ struct cred { > > struct key *thread_keyring; /* keyring private to > > this thread */ > > struct key *request_key_auth; /* assumed > > request_key authority */ > > #endif > > +#ifdef CONFIG_FILE_LOCKING > > + void *lease_breaker; /* identify NFS client > > breaking a delegation */ > > +#endif > > #ifdef CONFIG_SECURITY > > void *security; /* subjective LSM > > security */ > > #endif > > Sorry, but ewww. > > Two reasons for that comment: > > (1) The cred struct may get retained long past where you expect if > it gets > attached to another process or a file descriptor. > > (2) The ->lease_breaker pointer needs lifetime management in > cred.c. It will > potentially get copied around and may need cleaning up. > > Can you stick your breaker identity in a key struct as Jeff > suggested? > Bruce, Do you really need to do more than just identify that this is a knfsd thread vs not a knfsd thread? I'm assuming that a knfsd thread will usually be in a position to recall delegations before it even initiates an operation on the inode in question, won't it? IOW: what if you were to modify the lease code to allow knfsd threads to return a "please ignore me, and proceed with the operation that triggered the lease break" reply, and then handle conflicts between NFS clients outside the lease callback code altogether? Cheers Trond -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 13:46 ` Trond Myklebust @ 2018-03-20 14:49 ` J. Bruce Fields 2018-03-20 15:13 ` Trond Myklebust 0 siblings, 1 reply; 19+ messages in thread From: J. Bruce Fields @ 2018-03-20 14:49 UTC (permalink / raw) To: Trond Myklebust; +Cc: bfields, dhowells, linux-nfs, linux-fsdevel On Tue, Mar 20, 2018 at 01:46:20PM +0000, Trond Myklebust wrote: > On Tue, 2018-03-20 at 13:35 +0000, David Howells wrote: > > J. Bruce Fields <bfields@redhat.com> wrote: > > > > > @@ -139,6 +139,9 @@ struct cred { > > > struct key *thread_keyring; /* keyring private to > > > this thread */ > > > struct key *request_key_auth; /* assumed > > > request_key authority */ > > > #endif > > > +#ifdef CONFIG_FILE_LOCKING > > > + void *lease_breaker; /* identify NFS client > > > breaking a delegation */ > > > +#endif > > > #ifdef CONFIG_SECURITY > > > void *security; /* subjective LSM > > > security */ > > > #endif > > > > Sorry, but ewww. > > > > Two reasons for that comment: > > > > (1) The cred struct may get retained long past where you expect if > > it gets > > attached to another process or a file descriptor. > > > > (2) The ->lease_breaker pointer needs lifetime management in > > cred.c. It will > > potentially get copied around and may need cleaning up. > > > > Can you stick your breaker identity in a key struct as Jeff > > suggested? > > > > Bruce, > > Do you really need to do more than just identify that this is a knfsd > thread vs not a knfsd thread? I'm assuming that a knfsd thread will > usually be in a position to recall delegations before it even initiates > an operation on the inode in question, won't it? I think it could. I'm reluctant: - Once we support write delegations, I think we end up having to do that before basically every operation on a inode. - I'd like this to make it easy for someone to extend delegation support to userspace eventually too. I'm not sure exactly how we'd identify self-conflicts in that case (struct files?), but anyway I'd rather this wasn't too nfsd-specific. That said, I'm still curious: > IOW: what if you were to modify the lease code to allow knfsd threads > to return a "please ignore me, and proceed with the operation that > triggered the lease break" reply, and then handle conflicts between NFS > clients outside the lease callback code altogether? So if you're a random bit of code, how would you recommend testing whether you're running in a knfsd thread? --b. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 14:49 ` J. Bruce Fields @ 2018-03-20 15:13 ` Trond Myklebust 2018-03-20 16:02 ` bfields 0 siblings, 1 reply; 19+ messages in thread From: Trond Myklebust @ 2018-03-20 15:13 UTC (permalink / raw) To: bfields; +Cc: bfields, linux-nfs, dhowells, linux-fsdevel On Tue, 2018-03-20 at 10:49 -0400, J. Bruce Fields wrote: > On Tue, Mar 20, 2018 at 01:46:20PM +0000, Trond Myklebust wrote: > > On Tue, 2018-03-20 at 13:35 +0000, David Howells wrote: > > > J. Bruce Fields <bfields@redhat.com> wrote: > > > > > > > @@ -139,6 +139,9 @@ struct cred { > > > > struct key *thread_keyring; /* keyring private > > > > to > > > > this thread */ > > > > struct key *request_key_auth; /* assumed > > > > request_key authority */ > > > > #endif > > > > +#ifdef CONFIG_FILE_LOCKING > > > > + void *lease_breaker; /* identify NFS > > > > client > > > > breaking a delegation */ > > > > +#endif > > > > #ifdef CONFIG_SECURITY > > > > void *security; /* subjective > > > > LSM > > > > security */ > > > > #endif > > > > > > Sorry, but ewww. > > > > > > Two reasons for that comment: > > > > > > (1) The cred struct may get retained long past where you expect > > > if > > > it gets > > > attached to another process or a file descriptor. > > > > > > (2) The ->lease_breaker pointer needs lifetime management in > > > cred.c. It will > > > potentially get copied around and may need cleaning up. > > > > > > Can you stick your breaker identity in a key struct as Jeff > > > suggested? > > > > > > > Bruce, > > > > Do you really need to do more than just identify that this is a > > knfsd > > thread vs not a knfsd thread? I'm assuming that a knfsd thread will > > usually be in a position to recall delegations before it even > > initiates > > an operation on the inode in question, won't it? > > I think it could. I'm reluctant: > > - Once we support write delegations, I think we end up having > to > do that before basically every operation on a inode. > - I'd like this to make it easy for someone to extend > delegation > support to userspace eventually too. I'm not sure exactly > how > we'd identify self-conflicts in that case (struct files?), > but > anyway I'd rather this wasn't too nfsd-specific. That's my point. A userspace application is going to have to do something like this anyway. It cannot install hooks in the kernel to perform elaborate tests, so it is going to have to rely on something like the struct file_lock 'fl_nspid' field in order to determine whether or not to apply a lease break. i.e.: the userspace rule should be to break the lease if and only if it is not owned by my process. > That said, I'm still curious: > > > IOW: what if you were to modify the lease code to allow knfsd > > threads > > to return a "please ignore me, and proceed with the operation that > > triggered the lease break" reply, and then handle conflicts between > > NFS > > clients outside the lease callback code altogether? > > So if you're a random bit of code, how would you recommend testing > whether you're running in a knfsd thread? Right now, the knfsd threads are regular kernel threads, with each thread appearing to userspace to be a process in its own right. Is there any reason why we could not assign them a group pid that would allow the fl_nspid mechanism to work (i.e. set task->group_leader)? -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 15:13 ` Trond Myklebust @ 2018-03-20 16:02 ` bfields 2018-09-06 19:40 ` bfields 0 siblings, 1 reply; 19+ messages in thread From: bfields @ 2018-03-20 16:02 UTC (permalink / raw) To: Trond Myklebust; +Cc: bfields, linux-nfs, dhowells, linux-fsdevel On Tue, Mar 20, 2018 at 03:13:47PM +0000, Trond Myklebust wrote: > On Tue, 2018-03-20 at 10:49 -0400, J. Bruce Fields wrote: > > On Tue, Mar 20, 2018 at 01:46:20PM +0000, Trond Myklebust wrote: > > > On Tue, 2018-03-20 at 13:35 +0000, David Howells wrote: > > > > J. Bruce Fields <bfields@redhat.com> wrote: > > > > > > > > > @@ -139,6 +139,9 @@ struct cred { > > > > > struct key *thread_keyring; /* keyring private > > > > > to > > > > > this thread */ > > > > > struct key *request_key_auth; /* assumed > > > > > request_key authority */ > > > > > #endif > > > > > +#ifdef CONFIG_FILE_LOCKING > > > > > + void *lease_breaker; /* identify NFS > > > > > client > > > > > breaking a delegation */ > > > > > +#endif > > > > > #ifdef CONFIG_SECURITY > > > > > void *security; /* subjective > > > > > LSM > > > > > security */ > > > > > #endif > > > > > > > > Sorry, but ewww. > > > > > > > > Two reasons for that comment: > > > > > > > > (1) The cred struct may get retained long past where you expect > > > > if > > > > it gets > > > > attached to another process or a file descriptor. > > > > > > > > (2) The ->lease_breaker pointer needs lifetime management in > > > > cred.c. It will > > > > potentially get copied around and may need cleaning up. > > > > > > > > Can you stick your breaker identity in a key struct as Jeff > > > > suggested? > > > > > > > > > > Bruce, > > > > > > Do you really need to do more than just identify that this is a > > > knfsd > > > thread vs not a knfsd thread? I'm assuming that a knfsd thread will > > > usually be in a position to recall delegations before it even > > > initiates > > > an operation on the inode in question, won't it? > > > > I think it could. I'm reluctant: > > > > - Once we support write delegations, I think we end up having > > to > > do that before basically every operation on a inode. > > - I'd like this to make it easy for someone to extend > > delegation > > support to userspace eventually too. I'm not sure exactly > > how > > we'd identify self-conflicts in that case (struct files?), > > but > > anyway I'd rather this wasn't too nfsd-specific. > > That's my point. A userspace application is going to have to do > something like this anyway. It cannot install hooks in the kernel to > perform elaborate tests, so it is going to have to rely on something > like the struct file_lock 'fl_nspid' field in order to determine > whether or not to apply a lease break. > > i.e.: the userspace rule should be to break the lease if and only if it > is not owned by my process. I was thinking of using struct file (whoops, not "struct files", sorry for the typo above) to avoid assuming too much about any userspace server's threading model. > > That said, I'm still curious: > > > > > IOW: what if you were to modify the lease code to allow knfsd > > > threads > > > to return a "please ignore me, and proceed with the operation that > > > triggered the lease break" reply, and then handle conflicts between > > > NFS > > > clients outside the lease callback code altogether? > > > > So if you're a random bit of code, how would you recommend testing > > whether you're running in a knfsd thread? > > Right now, the knfsd threads are regular kernel threads, with each > thread appearing to userspace to be a process in its own right. > Is there any reason why we could not assign them a group pid that would > allow the fl_nspid mechanism to work (i.e. set task->group_leader)? Huh, OK, I hadn't thought about that. --b. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 16:02 ` bfields @ 2018-09-06 19:40 ` bfields 0 siblings, 0 replies; 19+ messages in thread From: bfields @ 2018-09-06 19:40 UTC (permalink / raw) To: Trond Myklebust; +Cc: bfields, linux-nfs, dhowells, linux-fsdevel On Tue, Mar 20, 2018 at 12:02:57PM -0400, bfields@fieldses.org wrote: > On Tue, Mar 20, 2018 at 03:13:47PM +0000, Trond Myklebust wrote: > > Right now, the knfsd threads are regular kernel threads, with each > > thread appearing to userspace to be a process in its own right. > > Is there any reason why we could not assign them a group pid that would > > allow the fl_nspid mechanism to work (i.e. set task->group_leader)? > > Huh, OK, I hadn't thought about that. Thinking about it: as far as I can tell that's not meant to be modified on a running thread, so we need to create the thread the right way--all the nfsd threads need to be children of the same thread. Currently we're using kthead_create() which makes them all children of kthreadd. I spent a little time looking into forking threads from existing nfsd threads and started to feel like I was duplicating a lot of kthread code. So now I wonder if it's possible to generalize kthreadd somehow. --b. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 10/10] nfsd: clients don't need to break their own delegations 2018-03-20 13:35 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations David Howells 2018-03-20 13:46 ` Trond Myklebust @ 2018-03-20 14:52 ` J. Bruce Fields 1 sibling, 0 replies; 19+ messages in thread From: J. Bruce Fields @ 2018-03-20 14:52 UTC (permalink / raw) To: David Howells; +Cc: J. Bruce Fields, linux-nfs, linux-fsdevel On Tue, Mar 20, 2018 at 01:35:51PM +0000, David Howells wrote: > J. Bruce Fields <bfields@redhat.com> wrote: > > > @@ -139,6 +139,9 @@ struct cred { > > struct key *thread_keyring; /* keyring private to this thread */ > > struct key *request_key_auth; /* assumed request_key authority */ > > #endif > > +#ifdef CONFIG_FILE_LOCKING > > + void *lease_breaker; /* identify NFS client breaking a delegation */ > > +#endif > > #ifdef CONFIG_SECURITY > > void *security; /* subjective LSM security */ > > #endif > > Sorry, but ewww. I'm sure you're right, but, to make sure I understand: > Two reasons for that comment: > > (1) The cred struct may get retained long past where you expect if it gets > attached to another process or a file descriptor. How would that happen in the case of a knfsd thread? > (2) The ->lease_breaker pointer needs lifetime management in cred.c. It will > potentially get copied around and may need cleaning up. Hm, OK. > Can you stick your breaker identity in a key struct as Jeff suggested? Probably so. I'm unfamiliar with the keyring code. How would you recommend doing that? --b. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-09-07 0:17 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-19 14:36 [PATCH 00/10] Eliminate delegation self-conflicts v2 J. Bruce Fields 2018-03-19 14:36 ` [PATCH 01/10] vfs: remove unnecessary fl_owner_t typedef J. Bruce Fields 2018-03-19 14:36 ` [PATCH 02/10] nfsd: simplify put of fi_deleg_file J. Bruce Fields 2018-03-19 14:36 ` [PATCH 03/10] nfsd: simplify nfs4_put_deleg_lease calls J. Bruce Fields 2018-03-19 14:36 ` [PATCH 04/10] nfsd4: set fl_owner to delegation, not file pointer J. Bruce Fields 2018-03-19 14:36 ` [PATCH 05/10] nfsd4: dp->dl_stid.sc_file doesn't need locking J. Bruce Fields 2018-03-19 14:36 ` [PATCH 06/10] nfsd: make nfs4_get_existing_delegation less confusing J. Bruce Fields 2018-03-19 14:36 ` [PATCH 07/10] nfsd: factor out common delegation-destruction code J. Bruce Fields 2018-03-19 14:36 ` [PATCH 08/10] nfsd: move sc_file assignment into alloc_init_deleg J. Bruce Fields 2018-03-19 14:36 ` [PATCH 09/10] nfsd: create a separate lease for each delegation J. Bruce Fields 2018-03-19 14:36 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations J. Bruce Fields 2018-03-20 13:10 ` [PATCH 00/10] Eliminate delegation self-conflicts v2 Jeff Layton 2018-03-20 13:35 ` [PATCH 10/10] nfsd: clients don't need to break their own delegations David Howells 2018-03-20 13:46 ` Trond Myklebust 2018-03-20 14:49 ` J. Bruce Fields 2018-03-20 15:13 ` Trond Myklebust 2018-03-20 16:02 ` bfields 2018-09-06 19:40 ` bfields 2018-03-20 14:52 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).