| Message ID | 20170207113554.GA30656@veci.piliscsaba.szeredi.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 07, 2017 at 12:35:54PM +0100, Miklos Szeredi wrote: > > Another thing: what guarantees that places in writepages-related paths > > where we store a reference into req->ff won't hit a request with already > > non-NULL ->ff? > > Well, it is set before being sent (queued onto queued_writes or queued on the > fuse device), but not when queued as secondary request onto an already in-flight > one. It looks okay to me. > void fuse_sync_release(struct fuse_file *ff, int flags) > { > - WARN_ON(atomic_read(&ff->count) > 1); > + WARN_ON(atomic_read(&ff->count) != 1); > fuse_prepare_release(ff, flags, FUSE_RELEASE); > - __set_bit(FR_FORCE, &ff->reserved_req->flags); > - __clear_bit(FR_BACKGROUND, &ff->reserved_req->flags); > - fuse_request_send(ff->fc, ff->reserved_req); > - fuse_put_request(ff->fc, ff->reserved_req); > - kfree(ff); > + fuse_file_put(ff, true); Umm... At the very least, that deserves a comment re "iput(NULL) is a no-op and since the refcount is 1 and everything's synchronous, we are fine with not doing igrab/iput here". There's enough mysteries in that code as it is... Speaking of mysteries - how can ->private_data ever be NULL in fuse_release_common()? AFAICS, it's only called from ->release() instances and those are only called after ->open() or ->atomic_open() on that struct file has returned 0. On the ->open() side, it means fuse_do_open() having returned 0; on ->atomic_open() one - fuse_create_open() having done the same. Neither is possible with ->private_data remaining NULL, and I don't see any places that would modify it afterwards... Another thing: am I right assuming that ff->nodeid will be the same for all ff over given inode (== get_node_id(inode))? What about ff->fh? Is that a per-open thing, or will it be identical for all opens of the same inode? -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 8, 2017 at 6:54 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Feb 07, 2017 at 12:35:54PM +0100, Miklos Szeredi wrote: >> > Another thing: what guarantees that places in writepages-related paths >> > where we store a reference into req->ff won't hit a request with already >> > non-NULL ->ff? >> >> Well, it is set before being sent (queued onto queued_writes or queued on the >> fuse device), but not when queued as secondary request onto an already in-flight >> one. It looks okay to me. > >> void fuse_sync_release(struct fuse_file *ff, int flags) >> { >> - WARN_ON(atomic_read(&ff->count) > 1); >> + WARN_ON(atomic_read(&ff->count) != 1); >> fuse_prepare_release(ff, flags, FUSE_RELEASE); >> - __set_bit(FR_FORCE, &ff->reserved_req->flags); >> - __clear_bit(FR_BACKGROUND, &ff->reserved_req->flags); >> - fuse_request_send(ff->fc, ff->reserved_req); >> - fuse_put_request(ff->fc, ff->reserved_req); >> - kfree(ff); >> + fuse_file_put(ff, true); > > Umm... At the very least, that deserves a comment re "iput(NULL) is a no-op > and since the refcount is 1 and everything's synchronous, we are fine with > not doing igrab/iput here". There's enough mysteries in that code as it is... Added comment. > Speaking of mysteries - how can ->private_data ever be NULL in > fuse_release_common()? AFAICS, it's only called from ->release() instances > and those are only called after ->open() or ->atomic_open() on that struct file > has returned 0. On the ->open() side, it means fuse_do_open() having returned > 0; on ->atomic_open() one - fuse_create_open() having done the same. Neither > is possible with ->private_data remaining NULL, and I don't see any places > that would modify it afterwards... Goes back to v2.6.15 (commit fd72faac95d7 "[PATCH] FUSE: atomic create+open"). Didn't make sense back then, and it doesn't now. Fixed. > Another thing: am I right assuming that ff->nodeid will be the same > for all ff over given inode (== get_node_id(inode))? Yes. Except for cuse, where it's zero. > What about ff->fh? > Is that a per-open thing, or will it be identical for all opens of the same > inode? A per-open thing (opaque identifier used by userspace fs to identify the open file) Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" 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/fuse/dir.c b/fs/fuse/dir.c index 811fd8929a18..e816166ce42f 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -473,7 +473,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, if (err) { fuse_sync_release(ff, flags); } else { - file->private_data = fuse_file_get(ff); + file->private_data = ff; fuse_finish_open(inode, file); } return err; diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 2401c5dabb2a..94cc4ab01f9a 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -58,7 +58,7 @@ struct fuse_file *fuse_file_alloc(struct fuse_conn *fc) } INIT_LIST_HEAD(&ff->write_entry); - atomic_set(&ff->count, 0); + atomic_set(&ff->count, 1); RB_CLEAR_NODE(&ff->polled_node); init_waitqueue_head(&ff->poll_wait); @@ -75,7 +75,7 @@ void fuse_file_free(struct fuse_file *ff) kfree(ff); } -struct fuse_file *fuse_file_get(struct fuse_file *ff) +static struct fuse_file *fuse_file_get(struct fuse_file *ff) { atomic_inc(&ff->count); return ff; @@ -100,6 +100,7 @@ static void fuse_file_put(struct fuse_file *ff, bool sync) iput(req->misc.release.inode); fuse_put_request(ff->fc, req); } else if (sync) { + __set_bit(FR_FORCE, &req->flags); __clear_bit(FR_BACKGROUND, &req->flags); fuse_request_send(ff->fc, req); iput(req->misc.release.inode); @@ -146,7 +147,7 @@ int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file, ff->open_flags &= ~FOPEN_DIRECT_IO; ff->nodeid = nodeid; - file->private_data = fuse_file_get(ff); + file->private_data = ff; return 0; } @@ -297,13 +298,9 @@ static int fuse_release(struct inode *inode, struct file *file) void fuse_sync_release(struct fuse_file *ff, int flags) { - WARN_ON(atomic_read(&ff->count) > 1); + WARN_ON(atomic_read(&ff->count) != 1); fuse_prepare_release(ff, flags, FUSE_RELEASE); - __set_bit(FR_FORCE, &ff->reserved_req->flags); - __clear_bit(FR_BACKGROUND, &ff->reserved_req->flags); - fuse_request_send(ff->fc, ff->reserved_req); - fuse_put_request(ff->fc, ff->reserved_req); - kfree(ff); + fuse_file_put(ff, true); } EXPORT_SYMBOL_GPL(fuse_sync_release); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 91307940c8ac..83f797271aef 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -732,7 +732,6 @@ void fuse_read_fill(struct fuse_req *req, struct file *file, int fuse_open_common(struct inode *inode, struct file *file, bool isdir); struct fuse_file *fuse_file_alloc(struct fuse_conn *fc); -struct fuse_file *fuse_file_get(struct fuse_file *ff); void fuse_file_free(struct fuse_file *ff); void fuse_finish_open(struct inode *inode, struct file *file);