Hi Christian, Jeff, Gao, Dominique,
Here are some additional patches for my netfs-lib tree:
(1) Fix __cachefiles_prepare_write() to correctly validate against the DIO
alignment.
(2) 9p: Fix initialisation of the netfs_inode so that i_size is set before
netfs_inode_init() is called.
(3) 9p: Do a couple of cleanups (remove a couple of unused vars and turn a
BUG_ON() into a warning).
(4) 9p: Always update remote_i_size, even if we're asked not to update
i_size in stat2inode.
(5) 9p: Return the amount written in preference to an error if we wrote
something.
David
The netfslib postings:
Link: https://lore.kernel.org/r/[email protected]/ # v1
Link: https://lore.kernel.org/r/[email protected]/ # v2
Link: https://lore.kernel.org/r/[email protected]/ # v3
Link: https://lore.kernel.org/r/[email protected]/ # v4
Link: https://lore.kernel.org/r/[email protected]/ # v5
David Howells (5):
cachefiles: Fix __cachefiles_prepare_write()
9p: Fix initialisation of netfs_inode for 9p
9p: Do a couple of cleanups
9p: Always update remote_i_size in stat2inode
9p: Use length of data written to the server in preference to error
fs/9p/v9fs_vfs.h | 1 +
fs/9p/vfs_addr.c | 24 ++++++++++++------------
fs/9p/vfs_inode.c | 6 +++---
fs/9p/vfs_inode_dotl.c | 7 ++++---
fs/cachefiles/io.c | 28 +++++++++++++++++-----------
5 files changed, 37 insertions(+), 29 deletions(-)
Fix __cachefiles_prepare_write() to correctly determine whether the
requested write will fit correctly with the DIO alignment.
Reported-by: Gao Xiang <[email protected]>
Signed-off-by: David Howells <[email protected]>
Tested-by: Yiqun Leng <[email protected]>
Tested-by: Jia Zhu <[email protected]>
cc: Jeff Layton <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
fs/cachefiles/io.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index bffffedce4a9..7529b40bc95a 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -522,16 +522,22 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
bool no_space_allocated_yet)
{
struct cachefiles_cache *cache = object->volume->cache;
- loff_t start = *_start, pos;
- size_t len = *_len, down;
+ unsigned long long start = *_start, pos;
+ size_t len = *_len;
int ret;
/* Round to DIO size */
- down = start - round_down(start, PAGE_SIZE);
- *_start = start - down;
- *_len = round_up(down + len, PAGE_SIZE);
- if (down < start || *_len > upper_len)
+ start = round_down(*_start, PAGE_SIZE);
+ if (start != *_start) {
+ kleave(" = -ENOBUFS [down]");
+ return -ENOBUFS;
+ }
+ if (*_len > upper_len) {
+ kleave(" = -ENOBUFS [up]");
return -ENOBUFS;
+ }
+
+ *_len = round_up(len, PAGE_SIZE);
/* We need to work out whether there's sufficient disk space to perform
* the write - but we can skip that check if we have space already
@@ -542,7 +548,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
pos = cachefiles_inject_read_error();
if (pos == 0)
- pos = vfs_llseek(file, *_start, SEEK_DATA);
+ pos = vfs_llseek(file, start, SEEK_DATA);
if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) {
if (pos == -ENXIO)
goto check_space; /* Unallocated tail */
@@ -550,7 +556,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
cachefiles_trace_seek_error);
return pos;
}
- if ((u64)pos >= (u64)*_start + *_len)
+ if (pos >= start + *_len)
goto check_space; /* Unallocated region */
/* We have a block that's at least partially filled - if we're low on
@@ -563,13 +569,13 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
pos = cachefiles_inject_read_error();
if (pos == 0)
- pos = vfs_llseek(file, *_start, SEEK_HOLE);
+ pos = vfs_llseek(file, start, SEEK_HOLE);
if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) {
trace_cachefiles_io_error(object, file_inode(file), pos,
cachefiles_trace_seek_error);
return pos;
}
- if ((u64)pos >= (u64)*_start + *_len)
+ if (pos >= start + *_len)
return 0; /* Fully allocated */
/* Partially allocated, but insufficient space: cull. */
@@ -577,7 +583,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
ret = cachefiles_inject_remove_error();
if (ret == 0)
ret = vfs_fallocate(file, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
- *_start, *_len);
+ start, *_len);
if (ret < 0) {
trace_cachefiles_io_error(object, file_inode(file), ret,
cachefiles_trace_fallocate_error);
The 9p filesystem is calling netfs_inode_init() in v9fs_init_inode() -
before the struct inode fields have been initialised from the obtained file
stats (ie. after v9fs_stat2inode*() has been called), but netfslib wants to
set a couple of its fields from i_size.
Reported-by: Marc Dionne <[email protected]>
Signed-off-by: David Howells <[email protected]>
Tested-by: Marc Dionne <[email protected]>
Tested-by: Dominique Martinet <[email protected]>
Acked-by: Dominique Martinet <[email protected]>
cc: Eric Van Hensbergen <[email protected]>
cc: Latchesar Ionkov <[email protected]>
cc: Dominique Martinet <[email protected]>
cc: Christian Schoenebeck <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
fs/9p/v9fs_vfs.h | 1 +
fs/9p/vfs_inode.c | 6 +++---
fs/9p/vfs_inode_dotl.c | 1 +
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 731e3d14b67d..0e8418066a48 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -42,6 +42,7 @@ struct inode *v9fs_alloc_inode(struct super_block *sb);
void v9fs_free_inode(struct inode *inode);
struct inode *v9fs_get_inode(struct super_block *sb, umode_t mode,
dev_t rdev);
+void v9fs_set_netfs_context(struct inode *inode);
int v9fs_init_inode(struct v9fs_session_info *v9ses,
struct inode *inode, umode_t mode, dev_t rdev);
void v9fs_evict_inode(struct inode *inode);
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index b66466e97459..32572982f72e 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -246,7 +246,7 @@ void v9fs_free_inode(struct inode *inode)
/*
* Set parameters for the netfs library
*/
-static void v9fs_set_netfs_context(struct inode *inode)
+void v9fs_set_netfs_context(struct inode *inode)
{
struct v9fs_inode *v9inode = V9FS_I(inode);
netfs_inode_init(&v9inode->netfs, &v9fs_req_ops, true);
@@ -326,8 +326,6 @@ int v9fs_init_inode(struct v9fs_session_info *v9ses,
err = -EINVAL;
goto error;
}
-
- v9fs_set_netfs_context(inode);
error:
return err;
@@ -359,6 +357,7 @@ struct inode *v9fs_get_inode(struct super_block *sb, umode_t mode, dev_t rdev)
iput(inode);
return ERR_PTR(err);
}
+ v9fs_set_netfs_context(inode);
return inode;
}
@@ -461,6 +460,7 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
goto error;
v9fs_stat2inode(st, inode, sb, 0);
+ v9fs_set_netfs_context(inode);
v9fs_cache_inode_get_cookie(inode);
unlock_new_inode(inode);
return inode;
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index e25fbc988f09..3505227e1704 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -128,6 +128,7 @@ static struct inode *v9fs_qid_iget_dotl(struct super_block *sb,
goto error;
v9fs_stat2inode_dotl(st, inode, 0);
+ v9fs_set_netfs_context(inode);
v9fs_cache_inode_get_cookie(inode);
retval = v9fs_get_acl(inode, fid);
if (retval)
In v9fs_upload_to_server(), we pass the error to netfslib to terminate the
subreq rather than the amount of data written - even if we did actually
write something.
Further, we assume that the write is always entirely done if successful -
but it might have been partially complete - as returned by
p9_client_write(), but we ignore that.
Fix this by indicating the amount written by preference and only returning
the error if we didn't write anything.
(We might want to return both in future if both are available as this
might be useful as to whether we retry or not.)
Suggested-by: Dominique Martinet <[email protected]>
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: David Howells <[email protected]>
cc: Eric Van Hensbergen <[email protected]>
cc: Latchesar Ionkov <[email protected]>
cc: Christian Schoenebeck <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
fs/9p/vfs_addr.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index f7f83eec3bcc..047855033d32 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -29,12 +29,11 @@
static void v9fs_upload_to_server(struct netfs_io_subrequest *subreq)
{
struct p9_fid *fid = subreq->rreq->netfs_priv;
- int err;
+ int err, len;
trace_netfs_sreq(subreq, netfs_sreq_trace_submit);
- p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
- netfs_write_subrequest_terminated(subreq, err < 0 ? err : subreq->len,
- false);
+ len = p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
+ netfs_write_subrequest_terminated(subreq, len ?: err, false);
}
static void v9fs_upload_to_server_worker(struct work_struct *work)
netfs: Rearrange netfs_io_subrequest to put request pointer first
Rearrange the netfs_io_subrequest struct to put the netfs_io_request
pointer (rreq) first. This then allows netfs_io_subrequest to be put in a
union with a pointer to a wrapper around netfs_io_request. This will be
useful in the future for cifs and maybe ceph.
Signed-off-by: David Howells <[email protected]>
cc: Steve French <[email protected]>
cc: Shyam Prasad N <[email protected]>
cc: Rohith Surabattula <[email protected]>
cc: Jeff Layton <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
include/linux/netfs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 852956aa3c4b..d3bac60fcd6f 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -204,8 +204,8 @@ struct netfs_cache_resources {
* the pages it points to can be relied on to exist for the duration.
*/
struct netfs_io_subrequest {
- struct work_struct work;
struct netfs_io_request *rreq; /* Supervising I/O request */
+ struct work_struct work;
struct list_head rreq_link; /* Link in rreq->subrequests */
struct iov_iter io_iter; /* Iterator for this subrequest */
loff_t start; /* Where to start the I/O */
Fix the proc/fs/fscache symlink to point to "netfs" not "../netfs".
Reported-by: Marc Dionne <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Jeff Layton <[email protected]>
cc: Christian Brauner <[email protected]>
cc: [email protected]
cc: [email protected]
---
fs/netfs/fscache_proc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/netfs/fscache_proc.c b/fs/netfs/fscache_proc.c
index ecd0d1edafaa..874d951bc390 100644
--- a/fs/netfs/fscache_proc.c
+++ b/fs/netfs/fscache_proc.c
@@ -16,7 +16,7 @@
*/
int __init fscache_proc_init(void)
{
- if (!proc_symlink("fs/fscache", NULL, "../netfs"))
+ if (!proc_symlink("fs/fscache", NULL, "netfs"))
goto error_sym;
if (!proc_create_seq("fs/netfs/caches", S_IFREG | 0444, NULL,
On Wed, Jan 03, 2024 at 02:59:25PM +0000, David Howells wrote:
> Fix __cachefiles_prepare_write() to correctly determine whether the
> requested write will fit correctly with the DIO alignment.
>
> Reported-by: Gao Xiang <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Tested-by: Yiqun Leng <[email protected]>
> Tested-by: Jia Zhu <[email protected]>
> cc: Jeff Layton <[email protected]>
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> cc: [email protected]
> ---
> fs/cachefiles/io.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index bffffedce4a9..7529b40bc95a 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -522,16 +522,22 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
> bool no_space_allocated_yet)
> {
> struct cachefiles_cache *cache = object->volume->cache;
> - loff_t start = *_start, pos;
> - size_t len = *_len, down;
> + unsigned long long start = *_start, pos;
> + size_t len = *_len;
> int ret;
>
> /* Round to DIO size */
> - down = start - round_down(start, PAGE_SIZE);
> - *_start = start - down;
> - *_len = round_up(down + len, PAGE_SIZE);
> - if (down < start || *_len > upper_len)
> + start = round_down(*_start, PAGE_SIZE);
> + if (start != *_start) {
> + kleave(" = -ENOBUFS [down]");
> + return -ENOBUFS;
> + }
> + if (*_len > upper_len) {
> + kleave(" = -ENOBUFS [up]");
> return -ENOBUFS;
> + }
> +
> + *_len = round_up(len, PAGE_SIZE);
>
> /* We need to work out whether there's sufficient disk space to perform
> * the write - but we can skip that check if we have space already
> @@ -542,7 +548,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
>
> pos = cachefiles_inject_read_error();
> if (pos == 0)
> - pos = vfs_llseek(file, *_start, SEEK_DATA);
> + pos = vfs_llseek(file, start, SEEK_DATA);
> if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) {
Hi David,
I realise these patches have been accepted, but I have a minor nit:
pos is now unsigned, and so cannot be less than zero.
Flagged by Smatch and Coccinelle.
> if (pos == -ENXIO)
> goto check_space; /* Unallocated tail */
> @@ -550,7 +556,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
> cachefiles_trace_seek_error);
> return pos;
> }
> - if ((u64)pos >= (u64)*_start + *_len)
> + if (pos >= start + *_len)
> goto check_space; /* Unallocated region */
>
> /* We have a block that's at least partially filled - if we're low on
> @@ -563,13 +569,13 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
>
> pos = cachefiles_inject_read_error();
> if (pos == 0)
> - pos = vfs_llseek(file, *_start, SEEK_HOLE);
> + pos = vfs_llseek(file, start, SEEK_HOLE);
> if (pos < 0 && pos >= (loff_t)-MAX_ERRNO) {
Ditto.
> trace_cachefiles_io_error(object, file_inode(file), pos,
> cachefiles_trace_seek_error);
> return pos;
> }
> - if ((u64)pos >= (u64)*_start + *_len)
> + if (pos >= start + *_len)
> return 0; /* Fully allocated */
>
> /* Partially allocated, but insufficient space: cull. */
> @@ -577,7 +583,7 @@ int __cachefiles_prepare_write(struct cachefiles_object *object,
> ret = cachefiles_inject_remove_error();
> if (ret == 0)
> ret = vfs_fallocate(file, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> - *_start, *_len);
> + start, *_len);
> if (ret < 0) {
> trace_cachefiles_io_error(object, file_inode(file), ret,
> cachefiles_trace_fallocate_error);
>