* [PATCH] fuse: enable caching of symlinks
@ 2018-09-27 13:52 Dan Schatzberg
2018-09-27 13:58 ` Miklos Szeredi
0 siblings, 1 reply; 5+ messages in thread
From: Dan Schatzberg @ 2018-09-27 13:52 UTC (permalink / raw)
To: Miklos Szeredi
Cc: tj, kernel-team, Dan Schatzberg, linux-fsdevel, linux-kernel
FUSE file reads are cached in the page cache, but symlink reads are
not. This patch enables FUSE READLINK operations to be cached which
can improve performance of some FUSE workloads.
In particular, I'm working on a FUSE filesystem for access to source
code and discovered that about a 10% improvement to build times is
achieved with this patch (there are a lot of symlinks in the source
tree).
Please provide feedback, I'm looking to flesh this out more.
Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
---
fs/fuse/dir.c | 70 +++++++++++++++++++++++++++++++--------------------
1 file changed, 43 insertions(+), 27 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 0979609d6eba..3c0a81ef5bca 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1398,38 +1398,46 @@ static int fuse_readdir(struct file *file, struct dir_context *ctx)
return err;
}
-static const char *fuse_get_link(struct dentry *dentry,
- struct inode *inode,
- struct delayed_call *done)
+static int fuse_symlink_readpage(struct file *file, struct page *page)
{
+ struct inode *inode = page->mapping->host;
struct fuse_conn *fc = get_fuse_conn(inode);
- FUSE_ARGS(args);
- char *link;
- ssize_t ret;
-
- if (!dentry)
- return ERR_PTR(-ECHILD);
+ struct fuse_req *req;
+ int err;
+ size_t num_read;
- link = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!link)
- return ERR_PTR(-ENOMEM);
+ err = -EIO;
+ if (is_bad_inode(inode))
+ goto out;
- args.in.h.opcode = FUSE_READLINK;
- args.in.h.nodeid = get_node_id(inode);
- args.out.argvar = 1;
- args.out.numargs = 1;
- args.out.args[0].size = PAGE_SIZE - 1;
- args.out.args[0].value = link;
- ret = fuse_simple_request(fc, &args);
- if (ret < 0) {
- kfree(link);
- link = ERR_PTR(ret);
- } else {
- link[ret] = '\0';
- set_delayed_call(done, kfree_link, link);
+ req = fuse_get_req(fc, 1);
+ if (IS_ERR(req)) {
+ err = PTR_ERR(req);
+ goto out;
}
+
+ req->out.page_zeroing = 1;
+ req->out.argpages = 1;
+ req->num_pages = 1;
+ req->pages[0] = page;
+ req->page_descs[0].length = PAGE_SIZE - 1;
+ req->in.h.opcode = FUSE_READLINK;
+ req->in.h.nodeid = get_node_id(inode);
+ req->out.argvar = 1;
+ req->out.numargs = 1;
+ req->out.args[0].size = PAGE_SIZE - 1;
+ fuse_request_send(fc, req);
+ num_read = req->out.args[0].size;
+ err = req->out.h.error;
+
+ if (!err)
+ SetPageUptodate(page);
+
+ fuse_put_request(fc, req);
fuse_invalidate_atime(inode);
- return link;
+out:
+ unlock_page(page);
+ return err;
}
static int fuse_dir_open(struct inode *inode, struct file *file)
@@ -1855,7 +1863,7 @@ static const struct inode_operations fuse_common_inode_operations = {
static const struct inode_operations fuse_symlink_inode_operations = {
.setattr = fuse_setattr,
- .get_link = fuse_get_link,
+ .get_link = page_get_link,
.getattr = fuse_getattr,
.listxattr = fuse_listxattr,
};
@@ -1871,7 +1879,15 @@ void fuse_init_dir(struct inode *inode)
inode->i_fop = &fuse_dir_operations;
}
+static const struct address_space_operations fuse_symlink_aops = {
+ .readpage = fuse_symlink_readpage,
+};
+
void fuse_init_symlink(struct inode *inode)
{
inode->i_op = &fuse_symlink_inode_operations;
+ inode->i_data.a_ops = &fuse_symlink_aops;
+ mapping_set_gfp_mask(inode->i_mapping,
+ mapping_gfp_constraint(inode->i_mapping,
+ ~(__GFP_FS | __GFP_HIGHMEM)));
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] fuse: enable caching of symlinks
2018-09-27 13:52 [PATCH] fuse: enable caching of symlinks Dan Schatzberg
@ 2018-09-27 13:58 ` Miklos Szeredi
2018-10-02 21:33 ` Dan Schatzberg
2018-10-09 14:29 ` Dan Schatzberg
0 siblings, 2 replies; 5+ messages in thread
From: Miklos Szeredi @ 2018-09-27 13:58 UTC (permalink / raw)
To: Dan Schatzberg; +Cc: Tejun Heo, kernel-team, linux-fsdevel, linux-kernel
On Thu, Sep 27, 2018 at 3:52 PM, Dan Schatzberg <dschatzberg@fb.com> wrote:
> FUSE file reads are cached in the page cache, but symlink reads are
> not. This patch enables FUSE READLINK operations to be cached which
> can improve performance of some FUSE workloads.
>
> In particular, I'm working on a FUSE filesystem for access to source
> code and discovered that about a 10% improvement to build times is
> achieved with this patch (there are a lot of symlinks in the source
> tree).
>
> Please provide feedback, I'm looking to flesh this out more.
Need to think about how/when to invalidate the cached symlink
contents. I think treating it like metadata (i.e. look at
attr_timeout for validity) would be the simplest.
Thanks,
Miklos
>
> Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
> ---
> fs/fuse/dir.c | 70 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0979609d6eba..3c0a81ef5bca 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1398,38 +1398,46 @@ static int fuse_readdir(struct file *file, struct dir_context *ctx)
> return err;
> }
>
> -static const char *fuse_get_link(struct dentry *dentry,
> - struct inode *inode,
> - struct delayed_call *done)
> +static int fuse_symlink_readpage(struct file *file, struct page *page)
> {
> + struct inode *inode = page->mapping->host;
> struct fuse_conn *fc = get_fuse_conn(inode);
> - FUSE_ARGS(args);
> - char *link;
> - ssize_t ret;
> -
> - if (!dentry)
> - return ERR_PTR(-ECHILD);
> + struct fuse_req *req;
> + int err;
> + size_t num_read;
>
> - link = kmalloc(PAGE_SIZE, GFP_KERNEL);
> - if (!link)
> - return ERR_PTR(-ENOMEM);
> + err = -EIO;
> + if (is_bad_inode(inode))
> + goto out;
>
> - args.in.h.opcode = FUSE_READLINK;
> - args.in.h.nodeid = get_node_id(inode);
> - args.out.argvar = 1;
> - args.out.numargs = 1;
> - args.out.args[0].size = PAGE_SIZE - 1;
> - args.out.args[0].value = link;
> - ret = fuse_simple_request(fc, &args);
> - if (ret < 0) {
> - kfree(link);
> - link = ERR_PTR(ret);
> - } else {
> - link[ret] = '\0';
> - set_delayed_call(done, kfree_link, link);
> + req = fuse_get_req(fc, 1);
> + if (IS_ERR(req)) {
> + err = PTR_ERR(req);
> + goto out;
> }
> +
> + req->out.page_zeroing = 1;
> + req->out.argpages = 1;
> + req->num_pages = 1;
> + req->pages[0] = page;
> + req->page_descs[0].length = PAGE_SIZE - 1;
> + req->in.h.opcode = FUSE_READLINK;
> + req->in.h.nodeid = get_node_id(inode);
> + req->out.argvar = 1;
> + req->out.numargs = 1;
> + req->out.args[0].size = PAGE_SIZE - 1;
> + fuse_request_send(fc, req);
> + num_read = req->out.args[0].size;
> + err = req->out.h.error;
> +
> + if (!err)
> + SetPageUptodate(page);
> +
> + fuse_put_request(fc, req);
> fuse_invalidate_atime(inode);
> - return link;
> +out:
> + unlock_page(page);
> + return err;
> }
>
> static int fuse_dir_open(struct inode *inode, struct file *file)
> @@ -1855,7 +1863,7 @@ static const struct inode_operations fuse_common_inode_operations = {
>
> static const struct inode_operations fuse_symlink_inode_operations = {
> .setattr = fuse_setattr,
> - .get_link = fuse_get_link,
> + .get_link = page_get_link,
> .getattr = fuse_getattr,
> .listxattr = fuse_listxattr,
> };
> @@ -1871,7 +1879,15 @@ void fuse_init_dir(struct inode *inode)
> inode->i_fop = &fuse_dir_operations;
> }
>
> +static const struct address_space_operations fuse_symlink_aops = {
> + .readpage = fuse_symlink_readpage,
> +};
> +
> void fuse_init_symlink(struct inode *inode)
> {
> inode->i_op = &fuse_symlink_inode_operations;
> + inode->i_data.a_ops = &fuse_symlink_aops;
> + mapping_set_gfp_mask(inode->i_mapping,
> + mapping_gfp_constraint(inode->i_mapping,
> + ~(__GFP_FS | __GFP_HIGHMEM)));
> }
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fuse: enable caching of symlinks
2018-09-27 13:58 ` Miklos Szeredi
@ 2018-10-02 21:33 ` Dan Schatzberg
2018-10-10 8:29 ` Miklos Szeredi
2018-10-09 14:29 ` Dan Schatzberg
1 sibling, 1 reply; 5+ messages in thread
From: Dan Schatzberg @ 2018-10-02 21:33 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Tejun Heo, Kernel Team, linux-fsdevel, linux-kernel
On 9/27/18, 9:59 AM, "Miklos Szeredi" <miklos@szeredi.hu> wrote:
> On Thu, Sep 27, 2018 at 3:52 PM, Dan Schatzberg <dschatzberg@fb.com> wrote:
>> FUSE file reads are cached in the page cache, but symlink reads are
>> not. This patch enables FUSE READLINK operations to be cached which
>> can improve performance of some FUSE workloads.
>>
>> In particular, I'm working on a FUSE filesystem for access to source
>> code and discovered that about a 10% improvement to build times is
>> achieved with this patch (there are a lot of symlinks in the source
>> tree).
>>
>> Please provide feedback, I'm looking to flesh this out more.
>
> Need to think about how/when to invalidate the cached symlink
> contents. I think treating it like metadata (i.e. look at
> attr_timeout for validity) would be the simplest.
>
> Thanks,
> Miklos
>
Any further thoughts on this? Just so I can confirm my understanding,
the attached patch will unconditionally cache READLINK responses
However, based on the entry timeout provided by the LOOKUP response,
Userspace can decide to invalidate this by providing a different inode on
a subsequent LOOKUP (e.g. during fuse_dentry_revalidate())
Would you like there to be a way to invalidate the cached symlink without
changing the inode via LOOKUP?
>>
>> Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
>> ---
>> fs/fuse/dir.c | 70 +++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 0979609d6eba..3c0a81ef5bca 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -1398,38 +1398,46 @@ static int fuse_readdir(struct file *file, struct dir_context *ctx)
>> return err;
>> }
>>
>> -static const char *fuse_get_link(struct dentry *dentry,
>> - struct inode *inode,
>> - struct delayed_call *done)
>> +static int fuse_symlink_readpage(struct file *file, struct page *page)
>> {
>> + struct inode *inode = page->mapping->host;
>> struct fuse_conn *fc = get_fuse_conn(inode);
>> - FUSE_ARGS(args);
>> - char *link;
>> - ssize_t ret;
>> -
>> - if (!dentry)
>> - return ERR_PTR(-ECHILD);
>> + struct fuse_req *req;
>> + int err;
>> + size_t num_read;
>>
>> - link = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> - if (!link)
>> - return ERR_PTR(-ENOMEM);
>> + err = -EIO;
>> + if (is_bad_inode(inode))
>> + goto out;
>>
>> - args.in.h.opcode = FUSE_READLINK;
>> - args.in.h.nodeid = get_node_id(inode);
>> - args.out.argvar = 1;
>> - args.out.numargs = 1;
>> - args.out.args[0].size = PAGE_SIZE - 1;
>> - args.out.args[0].value = link;
>> - ret = fuse_simple_request(fc, &args);
>> - if (ret < 0) {
>> - kfree(link);
>> - link = ERR_PTR(ret);
>> - } else {
>> - link[ret] = '\0';
>> - set_delayed_call(done, kfree_link, link);
>> + req = fuse_get_req(fc, 1);
>> + if (IS_ERR(req)) {
>> + err = PTR_ERR(req);
>> + goto out;
>> }
>> +
>> + req->out.page_zeroing = 1;
>> + req->out.argpages = 1;
>> + req->num_pages = 1;
>> + req->pages[0] = page;
>> + req->page_descs[0].length = PAGE_SIZE - 1;
>> + req->in.h.opcode = FUSE_READLINK;
>> + req->in.h.nodeid = get_node_id(inode);
>> + req->out.argvar = 1;
>> + req->out.numargs = 1;
>> + req->out.args[0].size = PAGE_SIZE - 1;
>> + fuse_request_send(fc, req);
>> + num_read = req->out.args[0].size;
>> + err = req->out.h.error;
>> +
>> + if (!err)
>> + SetPageUptodate(page);
>> +
>> + fuse_put_request(fc, req);
>> fuse_invalidate_atime(inode);
>> - return link;
>> +out:
>> + unlock_page(page);
>> + return err;
>> }
>>
>> static int fuse_dir_open(struct inode *inode, struct file *file)
>> @@ -1855,7 +1863,7 @@ static const struct inode_operations fuse_common_inode_operations = {
>>
>> static const struct inode_operations fuse_symlink_inode_operations = {
>> .setattr = fuse_setattr,
>> - .get_link = fuse_get_link,
>> + .get_link = page_get_link,
>> .getattr = fuse_getattr,
>> .listxattr = fuse_listxattr,
>> };
>> @@ -1871,7 +1879,15 @@ void fuse_init_dir(struct inode *inode)
>> inode->i_fop = &fuse_dir_operations;
>> }
>>
>> +static const struct address_space_operations fuse_symlink_aops = {
>> + .readpage = fuse_symlink_readpage,
>> +};
>> +
>> void fuse_init_symlink(struct inode *inode)
>> {
>> inode->i_op = &fuse_symlink_inode_operations;
>> + inode->i_data.a_ops = &fuse_symlink_aops;
>> + mapping_set_gfp_mask(inode->i_mapping,
>> + mapping_gfp_constraint(inode->i_mapping,
>> + ~(__GFP_FS | __GFP_HIGHMEM)));
>> }
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fuse: enable caching of symlinks
2018-10-02 21:33 ` Dan Schatzberg
@ 2018-10-10 8:29 ` Miklos Szeredi
0 siblings, 0 replies; 5+ messages in thread
From: Miklos Szeredi @ 2018-10-10 8:29 UTC (permalink / raw)
To: Dan Schatzberg; +Cc: Tejun Heo, Kernel Team, linux-fsdevel, linux-kernel
On Tue, Oct 2, 2018 at 11:33 PM, Dan Schatzberg <dschatzberg@fb.com> wrote:
> On 9/27/18, 9:59 AM, "Miklos Szeredi" <miklos@szeredi.hu> wrote:
>
>> On Thu, Sep 27, 2018 at 3:52 PM, Dan Schatzberg <dschatzberg@fb.com> wrote:
>>> FUSE file reads are cached in the page cache, but symlink reads are
>>> not. This patch enables FUSE READLINK operations to be cached which
>>> can improve performance of some FUSE workloads.
>>>
>>> In particular, I'm working on a FUSE filesystem for access to source
>>> code and discovered that about a 10% improvement to build times is
>>> achieved with this patch (there are a lot of symlinks in the source
>>> tree).
>>>
>>> Please provide feedback, I'm looking to flesh this out more.
>>
>> Need to think about how/when to invalidate the cached symlink
>> contents. I think treating it like metadata (i.e. look at
>> attr_timeout for validity) would be the simplest.
>>
>> Thanks,
>> Miklos
>>
>
> Any further thoughts on this? Just so I can confirm my understanding,
> the attached patch will unconditionally cache READLINK responses
> However, based on the entry timeout provided by the LOOKUP response,
> Userspace can decide to invalidate this by providing a different inode on
> a subsequent LOOKUP (e.g. during fuse_dentry_revalidate())
Indeed.
> Would you like there to be a way to invalidate the cached symlink without
> changing the inode via LOOKUP?
No, that would make no sense, since symlink contents are not mutable.
This was just a braino on my part.
I still worry about backward compatibility, though. So we should add
a flag to negotiate symlink caching in with the INIT request.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fuse: enable caching of symlinks
2018-09-27 13:58 ` Miklos Szeredi
2018-10-02 21:33 ` Dan Schatzberg
@ 2018-10-09 14:29 ` Dan Schatzberg
1 sibling, 0 replies; 5+ messages in thread
From: Dan Schatzberg @ 2018-10-09 14:29 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Tejun Heo, Kernel Team, linux-fsdevel, linux-kernel
On 9/27/18, 9:59 AM, "Miklos Szeredi" <miklos@szeredi.hu> wrote:
> On Thu, Sep 27, 2018 at 3:52 PM, Dan Schatzberg <dschatzberg@fb.com> wrote:
>> FUSE file reads are cached in the page cache, but symlink reads are
>> not. This patch enables FUSE READLINK operations to be cached which
>> can improve performance of some FUSE workloads.
>>
>> In particular, I'm working on a FUSE filesystem for access to source
>> code and discovered that about a 10% improvement to build times is
>> achieved with this patch (there are a lot of symlinks in the source
>> tree).
>>
>> Please provide feedback, I'm looking to flesh this out more.
>
> Need to think about how/when to invalidate the cached symlink
> contents. I think treating it like metadata (i.e. look at
> attr_timeout for validity) would be the simplest.
>
> Thanks,
> Miklos
>
Any further thoughts on this? Just so I can confirm my understanding,
the attached patch will unconditionally cache READLINK responses
However, based on the entry timeout provided by the LOOKUP response,
Userspace can decide to invalidate this by providing a different inode on
a subsequent LOOKUP (e.g. during fuse_dentry_revalidate())
Would you like there to be a way to invalidate the cached symlink without
changing the inode via LOOKUP?
>> Signed-off-by: Dan Schatzberg <dschatzberg@fb.com>
>> ---
>> fs/fuse/dir.c | 70 +++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index 0979609d6eba..3c0a81ef5bca 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -1398,38 +1398,46 @@ static int fuse_readdir(struct file *file, struct dir_context *ctx)
>> return err;
>> }
>>
>> -static const char *fuse_get_link(struct dentry *dentry,
>> - struct inode *inode,
>> - struct delayed_call *done)
>> +static int fuse_symlink_readpage(struct file *file, struct page *page)
>> {
>> + struct inode *inode = page->mapping->host;
>> struct fuse_conn *fc = get_fuse_conn(inode);
>> - FUSE_ARGS(args);
>> - char *link;
>> - ssize_t ret;
>> -
>> - if (!dentry)
>> - return ERR_PTR(-ECHILD);
>> + struct fuse_req *req;
>> + int err;
>> + size_t num_read;
>>
>> - link = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> - if (!link)
>> - return ERR_PTR(-ENOMEM);
>> + err = -EIO;
>> + if (is_bad_inode(inode))
>> + goto out;
>>
>> - args.in.h.opcode = FUSE_READLINK;
>> - args.in.h.nodeid = get_node_id(inode);
>> - args.out.argvar = 1;
>> - args.out.numargs = 1;
>> - args.out.args[0].size = PAGE_SIZE - 1;
>> - args.out.args[0].value = link;
>> - ret = fuse_simple_request(fc, &args);
>> - if (ret < 0) {
>> - kfree(link);
>> - link = ERR_PTR(ret);
>> - } else {
>> - link[ret] = '\0';
>> - set_delayed_call(done, kfree_link, link);
>> + req = fuse_get_req(fc, 1);
>> + if (IS_ERR(req)) {
>> + err = PTR_ERR(req);
>> + goto out;
>> }
>> +
>> + req->out.page_zeroing = 1;
>> + req->out.argpages = 1;
>> + req->num_pages = 1;
>> + req->pages[0] = page;
>> + req->page_descs[0].length = PAGE_SIZE - 1;
>> + req->in.h.opcode = FUSE_READLINK;
>> + req->in.h.nodeid = get_node_id(inode);
>> + req->out.argvar = 1;
>> + req->out.numargs = 1;
>> + req->out.args[0].size = PAGE_SIZE - 1;
>> + fuse_request_send(fc, req);
>> + num_read = req->out.args[0].size;
>> + err = req->out.h.error;
>> +
>> + if (!err)
>> + SetPageUptodate(page);
>> +
>> + fuse_put_request(fc, req);
>> fuse_invalidate_atime(inode);
>> - return link;
>> +out:
>> + unlock_page(page);
>> + return err;
>> }
>>
>> static int fuse_dir_open(struct inode *inode, struct file *file)
>> @@ -1855,7 +1863,7 @@ static const struct inode_operations fuse_common_inode_operations = {
>>
>> static const struct inode_operations fuse_symlink_inode_operations = {
>> .setattr = fuse_setattr,
>> - .get_link = fuse_get_link,
>> + .get_link = page_get_link,
>> .getattr = fuse_getattr,
>> .listxattr = fuse_listxattr,
>> };
>> @@ -1871,7 +1879,15 @@ void fuse_init_dir(struct inode *inode)
>> inode->i_fop = &fuse_dir_operations;
>> }
>>
>> +static const struct address_space_operations fuse_symlink_aops = {
>> + .readpage = fuse_symlink_readpage,
>> +};
>> +
>> void fuse_init_symlink(struct inode *inode)
>> {
>> inode->i_op = &fuse_symlink_inode_operations;
>> + inode->i_data.a_ops = &fuse_symlink_aops;
>> + mapping_set_gfp_mask(inode->i_mapping,
>> + mapping_gfp_constraint(inode->i_mapping,
>> + ~(__GFP_FS | __GFP_HIGHMEM)));
>> }
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-10 8:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-27 13:52 [PATCH] fuse: enable caching of symlinks Dan Schatzberg
2018-09-27 13:58 ` Miklos Szeredi
2018-10-02 21:33 ` Dan Schatzberg
2018-10-10 8:29 ` Miklos Szeredi
2018-10-09 14:29 ` Dan Schatzberg
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).