* [PULL 2/9] 9pfs: fix FD leak and reduce latency of v9fs_reclaim_fd()
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 7/9] tests/9p: add 'Tsetattr' request to test client Christian Schoenebeck
` (8 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
This patch fixes two different bugs in v9fs_reclaim_fd():
1. Reduce latency:
This function calls v9fs_co_close() and v9fs_co_closedir() in a loop. Each
one of the calls adds two thread hops (between main thread and a fs driver
background thread). Each thread hop adds latency, which sums up in
function's loop to a significant duration.
Reduce overall latency by open coding what v9fs_co_close() and
v9fs_co_closedir() do, executing those and the loop itself altogether in
only one background thread block, hence reducing the total amount of
thread hops to only two.
2. Fix file descriptor leak:
The existing code called v9fs_co_close() and v9fs_co_closedir() to close
file descriptors. Both functions check right at the beginning if the 9p
request was cancelled:
if (v9fs_request_cancelled(pdu)) {
return -EINTR;
}
So if client sent a 'Tflush' message, v9fs_co_close() / v9fs_co_closedir()
returned without having closed the file descriptor and v9fs_reclaim_fd()
subsequently freed the FID without its file descriptor being closed, hence
leaking those file descriptors.
This 2nd bug is fixed by this patch as well by open coding v9fs_co_close()
and v9fs_co_closedir() inside of v9fs_reclaim_fd() and not performing the
v9fs_request_cancelled(pdu) check there.
Fixes: 7a46274529c ('hw/9pfs: Add file descriptor reclaim support')
Fixes: bccacf6c792 ('hw/9pfs: Implement TFLUSH operation')
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <5747469d3f039c53147e850b456943a1d4b5485c.1741339452.git.qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4f9c2dde9c..80b190ff5b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -434,6 +434,8 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
V9fsFidState *f;
GHashTableIter iter;
gpointer fid;
+ int err;
+ int nclosed = 0;
/* prevent multiple coroutines running this function simultaniously */
if (s->reclaiming) {
@@ -446,10 +448,10 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
QSLIST_HEAD(, V9fsFidState) reclaim_list =
QSLIST_HEAD_INITIALIZER(reclaim_list);
+ /* Pick FIDs to be closed, collect them on reclaim_list. */
while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &f)) {
/*
- * Unlink fids cannot be reclaimed. Check
- * for them and skip them. Also skip fids
+ * Unlinked fids cannot be reclaimed, skip those, and also skip fids
* currently being operated on.
*/
if (f->ref || f->flags & FID_NON_RECLAIMABLE) {
@@ -499,17 +501,26 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
}
}
/*
- * Now close the fid in reclaim list. Free them if they
- * are already clunked.
+ * Close the picked FIDs altogether on a background I/O driver thread. Do
+ * this all at once to keep latency (i.e. amount of thread hops between main
+ * thread <-> fs driver background thread) as low as possible.
*/
+ v9fs_co_run_in_worker({
+ QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
+ err = (f->fid_type == P9_FID_DIR) ?
+ s->ops->closedir(&s->ctx, &f->fs_reclaim) :
+ s->ops->close(&s->ctx, &f->fs_reclaim);
+ if (!err) {
+ /* total_open_fd must only be mutated on main thread */
+ nclosed++;
+ }
+ }
+ });
+ total_open_fd -= nclosed;
+ /* Free the closed FIDs. */
while (!QSLIST_EMPTY(&reclaim_list)) {
f = QSLIST_FIRST(&reclaim_list);
QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
- if (f->fid_type == P9_FID_FILE) {
- v9fs_co_close(pdu, &f->fs_reclaim);
- } else if (f->fid_type == P9_FID_DIR) {
- v9fs_co_closedir(pdu, &f->fs_reclaim);
- }
/*
* Now drop the fid reference, free it
* if clunked.
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 7/9] tests/9p: add 'Tsetattr' request to test client
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
2025-05-05 9:50 ` [PULL 2/9] 9pfs: fix FD leak and reduce latency of v9fs_reclaim_fd() Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-07-10 13:30 ` Peter Maydell
2025-05-05 9:50 ` [PULL 5/9] 9pfs: Introduce ftruncate file op Christian Schoenebeck
` (7 subsequent siblings)
9 siblings, 1 reply; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
Add and implement functions to 9pfs test client for sending a 9p2000.L
'Tsetattr' request and receiving its 'Rsetattr' response counterpart.
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <20250312152933.383967-6-groug@kaod.org>
---
tests/qtest/libqos/virtio-9p-client.c | 49 +++++++++++++++++++++++++++
tests/qtest/libqos/virtio-9p-client.h | 34 +++++++++++++++++++
tests/qtest/virtio-9p-test.c | 1 +
3 files changed, 84 insertions(+)
diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
index 98b77db51d..6ab4501c6e 100644
--- a/tests/qtest/libqos/virtio-9p-client.c
+++ b/tests/qtest/libqos/virtio-9p-client.c
@@ -557,6 +557,55 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
v9fs_req_free(req);
}
+/*
+ * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
+ * atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
+ */
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
+{
+ P9Req *req;
+ uint32_t err;
+
+ g_assert(opt.client);
+
+ req = v9fs_req_init(
+ opt.client, 4/*fid*/ + 4/*valid*/ + 4/*mode*/ + 4/*uid*/ + 4/*gid*/ +
+ 8/*size*/ + 8/*atime_sec*/ + 8/*atime_nsec*/ + 8/*mtime_sec*/ +
+ 8/*mtime_nsec*/, P9_TSETATTR, opt.tag
+ );
+ v9fs_uint32_write(req, opt.fid);
+ v9fs_uint32_write(req, (uint32_t) opt.attr.valid);
+ v9fs_uint32_write(req, opt.attr.mode);
+ v9fs_uint32_write(req, opt.attr.uid);
+ v9fs_uint32_write(req, opt.attr.gid);
+ v9fs_uint64_write(req, opt.attr.size);
+ v9fs_uint64_write(req, opt.attr.atime_sec);
+ v9fs_uint64_write(req, opt.attr.atime_nsec);
+ v9fs_uint64_write(req, opt.attr.mtime_sec);
+ v9fs_uint64_write(req, opt.attr.mtime_nsec);
+ v9fs_req_send(req);
+
+ if (!opt.requestOnly) {
+ v9fs_req_wait_for_reply(req, NULL);
+ if (opt.expectErr) {
+ v9fs_rlerror(req, &err);
+ g_assert_cmpint(err, ==, opt.expectErr);
+ } else {
+ v9fs_rsetattr(req);
+ }
+ req = NULL; /* request was freed */
+ }
+
+ return (TSetAttrRes) { .req = req };
+}
+
+/* size[4] Rsetattr tag[2] */
+void v9fs_rsetattr(P9Req *req)
+{
+ v9fs_req_recv(req, P9_RSETATTR);
+ v9fs_req_free(req);
+}
+
/* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
TReadDirRes v9fs_treaddir(TReadDirOpt opt)
{
diff --git a/tests/qtest/libqos/virtio-9p-client.h b/tests/qtest/libqos/virtio-9p-client.h
index 78228eb97d..e3221a3104 100644
--- a/tests/qtest/libqos/virtio-9p-client.h
+++ b/tests/qtest/libqos/virtio-9p-client.h
@@ -65,6 +65,16 @@ typedef struct v9fs_attr {
#define P9_GETATTR_BASIC 0x000007ffULL /* Mask for fields up to BLOCKS */
#define P9_GETATTR_ALL 0x00003fffULL /* Mask for ALL fields */
+#define P9_SETATTR_MODE 0x00000001UL
+#define P9_SETATTR_UID 0x00000002UL
+#define P9_SETATTR_GID 0x00000004UL
+#define P9_SETATTR_SIZE 0x00000008UL
+#define P9_SETATTR_ATIME 0x00000010UL
+#define P9_SETATTR_MTIME 0x00000020UL
+#define P9_SETATTR_CTIME 0x00000040UL
+#define P9_SETATTR_ATIME_SET 0x00000080UL
+#define P9_SETATTR_MTIME_SET 0x00000100UL
+
struct V9fsDirent {
v9fs_qid qid;
uint64_t offset;
@@ -182,6 +192,28 @@ typedef struct TGetAttrRes {
P9Req *req;
} TGetAttrRes;
+/* options for 'Tsetattr' 9p request */
+typedef struct TSetAttrOpt {
+ /* 9P client being used (mandatory) */
+ QVirtio9P *client;
+ /* user supplied tag number being returned with response (optional) */
+ uint16_t tag;
+ /* file ID of file/dir whose attributes shall be modified (required) */
+ uint32_t fid;
+ /* new attribute values to be set by 9p server */
+ v9fs_attr attr;
+ /* only send Tsetattr request but not wait for a reply? (optional) */
+ bool requestOnly;
+ /* do we expect an Rlerror response, if yes which error code? (optional) */
+ uint32_t expectErr;
+} TSetAttrOpt;
+
+/* result of 'Tsetattr' 9p request */
+typedef struct TSetAttrRes {
+ /* if requestOnly was set: request object for further processing */
+ P9Req *req;
+} TSetAttrRes;
+
/* options for 'Treaddir' 9p request */
typedef struct TReadDirOpt {
/* 9P client being used (mandatory) */
@@ -470,6 +502,8 @@ TWalkRes v9fs_twalk(TWalkOpt opt);
void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid);
TGetAttrRes v9fs_tgetattr(TGetAttrOpt);
void v9fs_rgetattr(P9Req *req, v9fs_attr *attr);
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt);
+void v9fs_rsetattr(P9Req *req);
TReadDirRes v9fs_treaddir(TReadDirOpt);
void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t *nentries,
struct V9fsDirent **entries);
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index ab3a12c816..f515a9bb15 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -20,6 +20,7 @@
#define tversion(...) v9fs_tversion((TVersionOpt) __VA_ARGS__)
#define tattach(...) v9fs_tattach((TAttachOpt) __VA_ARGS__)
#define tgetattr(...) v9fs_tgetattr((TGetAttrOpt) __VA_ARGS__)
+#define tsetattr(...) v9fs_tsetattr((TSetAttrOpt) __VA_ARGS__)
#define treaddir(...) v9fs_treaddir((TReadDirOpt) __VA_ARGS__)
#define tlopen(...) v9fs_tlopen((TLOpenOpt) __VA_ARGS__)
#define twrite(...) v9fs_twrite((TWriteOpt) __VA_ARGS__)
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 7/9] tests/9p: add 'Tsetattr' request to test client
2025-05-05 9:50 ` [PULL 7/9] tests/9p: add 'Tsetattr' request to test client Christian Schoenebeck
@ 2025-07-10 13:30 ` Peter Maydell
2025-07-10 14:11 ` Christian Schoenebeck
0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2025-07-10 13:30 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Greg Kurz
On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> Add and implement functions to 9pfs test client for sending a 9p2000.L
> 'Tsetattr' request and receiving its 'Rsetattr' response counterpart.
>
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Message-Id: <20250312152933.383967-6-groug@kaod.org>
> ---
> tests/qtest/libqos/virtio-9p-client.c | 49 +++++++++++++++++++++++++++
> tests/qtest/libqos/virtio-9p-client.h | 34 +++++++++++++++++++
> tests/qtest/virtio-9p-test.c | 1 +
> 3 files changed, 84 insertions(+)
>
> diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
> index 98b77db51d..6ab4501c6e 100644
> --- a/tests/qtest/libqos/virtio-9p-client.c
> +++ b/tests/qtest/libqos/virtio-9p-client.c
> @@ -557,6 +557,55 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
> v9fs_req_free(req);
> }
>
> +/*
> + * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
> + * atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
> + */
> +TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
> +{
> + P9Req *req;
> + uint32_t err;
Hi -- Coverity warns (CID 1609751) that this function
passes by value an argument which is a 184 byte struct.
Is this intentional? Can we instead pass a pointer to the
struct?
This is only a test program and 184 bytes is not super
enormous, so if this would be painful to avoid we can mark
the coverity report as a false positive.
thanks
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PULL 7/9] tests/9p: add 'Tsetattr' request to test client
2025-07-10 13:30 ` Peter Maydell
@ 2025-07-10 14:11 ` Christian Schoenebeck
2025-07-10 14:17 ` Peter Maydell
0 siblings, 1 reply; 14+ messages in thread
From: Christian Schoenebeck @ 2025-07-10 14:11 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
On Thursday, July 10, 2025 3:30:22 PM CEST Peter Maydell wrote:
> On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
> >
> > Add and implement functions to 9pfs test client for sending a 9p2000.L
> > 'Tsetattr' request and receiving its 'Rsetattr' response counterpart.
> >
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Message-Id: <20250312152933.383967-6-groug@kaod.org>
> > ---
> > tests/qtest/libqos/virtio-9p-client.c | 49 +++++++++++++++++++++++++++
> > tests/qtest/libqos/virtio-9p-client.h | 34 +++++++++++++++++++
> > tests/qtest/virtio-9p-test.c | 1 +
> > 3 files changed, 84 insertions(+)
> >
> > diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
> > index 98b77db51d..6ab4501c6e 100644
> > --- a/tests/qtest/libqos/virtio-9p-client.c
> > +++ b/tests/qtest/libqos/virtio-9p-client.c
> > @@ -557,6 +557,55 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
> > v9fs_req_free(req);
> > }
> >
> > +/*
> > + * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
> > + * atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
> > + */
> > +TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
> > +{
> > + P9Req *req;
> > + uint32_t err;
>
>
> Hi -- Coverity warns (CID 1609751) that this function
> passes by value an argument which is a 184 byte struct.
> Is this intentional?
Hi Peter!
Yes, that was intentional. It follows the same coding pattern of the 9p test
cases to hack named function arguments into C:
someFunc({ .argC = 3, .argH = "foo", .argX = 1 });
That saves a lot of code and makes callers better readable, because some test
case just needs to pass a value for argument A and C, another test might need
to pass arguments H, X and Y, and so on.
Before we had numerous function variations for the same thing, just with
different argument permutations. Now it's only one function per purpose.
> Can we instead pass a pointer to the
> struct?
>
> This is only a test program and 184 bytes is not super
> enormous, so if this would be painful to avoid we can mark
> the coverity report as a false positive.
Well, it would be possible to change this to a pointer, patch below in any
case. Personally I would just mark this as a false positive though. It's not a
bug and the resulting binary would probably be identical. No hard opinion
though.
/Christian
diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
index 6ab4501c6e..f0b009645d 100644
--- a/tests/qtest/libqos/virtio-9p-client.c
+++ b/tests/qtest/libqos/virtio-9p-client.c
@@ -561,35 +561,35 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
* size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
* atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
*/
-TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt *opt)
{
P9Req *req;
uint32_t err;
- g_assert(opt.client);
+ g_assert(opt->client);
req = v9fs_req_init(
- opt.client, 4/*fid*/ + 4/*valid*/ + 4/*mode*/ + 4/*uid*/ + 4/*gid*/ +
+ opt->client, 4/*fid*/ + 4/*valid*/ + 4/*mode*/ + 4/*uid*/ + 4/*gid*/ +
8/*size*/ + 8/*atime_sec*/ + 8/*atime_nsec*/ + 8/*mtime_sec*/ +
- 8/*mtime_nsec*/, P9_TSETATTR, opt.tag
+ 8/*mtime_nsec*/, P9_TSETATTR, opt->tag
);
- v9fs_uint32_write(req, opt.fid);
- v9fs_uint32_write(req, (uint32_t) opt.attr.valid);
- v9fs_uint32_write(req, opt.attr.mode);
- v9fs_uint32_write(req, opt.attr.uid);
- v9fs_uint32_write(req, opt.attr.gid);
- v9fs_uint64_write(req, opt.attr.size);
- v9fs_uint64_write(req, opt.attr.atime_sec);
- v9fs_uint64_write(req, opt.attr.atime_nsec);
- v9fs_uint64_write(req, opt.attr.mtime_sec);
- v9fs_uint64_write(req, opt.attr.mtime_nsec);
+ v9fs_uint32_write(req, opt->fid);
+ v9fs_uint32_write(req, (uint32_t) opt->attr.valid);
+ v9fs_uint32_write(req, opt->attr.mode);
+ v9fs_uint32_write(req, opt->attr.uid);
+ v9fs_uint32_write(req, opt->attr.gid);
+ v9fs_uint64_write(req, opt->attr.size);
+ v9fs_uint64_write(req, opt->attr.atime_sec);
+ v9fs_uint64_write(req, opt->attr.atime_nsec);
+ v9fs_uint64_write(req, opt->attr.mtime_sec);
+ v9fs_uint64_write(req, opt->attr.mtime_nsec);
v9fs_req_send(req);
- if (!opt.requestOnly) {
+ if (!opt->requestOnly) {
v9fs_req_wait_for_reply(req, NULL);
- if (opt.expectErr) {
+ if (opt->expectErr) {
v9fs_rlerror(req, &err);
- g_assert_cmpint(err, ==, opt.expectErr);
+ g_assert_cmpint(err, ==, opt->expectErr);
} else {
v9fs_rsetattr(req);
}
diff --git a/tests/qtest/libqos/virtio-9p-client.h b/tests/qtest/libqos/virtio-9p-client.h
index e3221a3104..4b55d7a56d 100644
--- a/tests/qtest/libqos/virtio-9p-client.h
+++ b/tests/qtest/libqos/virtio-9p-client.h
@@ -502,7 +502,7 @@ TWalkRes v9fs_twalk(TWalkOpt opt);
void v9fs_rwalk(P9Req *req, uint16_t *nwqid, v9fs_qid **wqid);
TGetAttrRes v9fs_tgetattr(TGetAttrOpt);
void v9fs_rgetattr(P9Req *req, v9fs_attr *attr);
-TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt);
+TSetAttrRes v9fs_tsetattr(TSetAttrOpt *opt);
void v9fs_rsetattr(P9Req *req);
TReadDirRes v9fs_treaddir(TReadDirOpt);
void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t *nentries,
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index ac38ccf595..4397c0738f 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -20,7 +20,7 @@
#define tversion(...) v9fs_tversion((TVersionOpt) __VA_ARGS__)
#define tattach(...) v9fs_tattach((TAttachOpt) __VA_ARGS__)
#define tgetattr(...) v9fs_tgetattr((TGetAttrOpt) __VA_ARGS__)
-#define tsetattr(...) v9fs_tsetattr((TSetAttrOpt) __VA_ARGS__)
+#define tsetattr(...) v9fs_tsetattr(&((TSetAttrOpt) __VA_ARGS__))
#define treaddir(...) v9fs_treaddir((TReadDirOpt) __VA_ARGS__)
#define tlopen(...) v9fs_tlopen((TLOpenOpt) __VA_ARGS__)
#define twrite(...) v9fs_twrite((TWriteOpt) __VA_ARGS__)
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 7/9] tests/9p: add 'Tsetattr' request to test client
2025-07-10 14:11 ` Christian Schoenebeck
@ 2025-07-10 14:17 ` Peter Maydell
0 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2025-07-10 14:17 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Greg Kurz
On Thu, 10 Jul 2025 at 15:11, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Thursday, July 10, 2025 3:30:22 PM CEST Peter Maydell wrote:
> > On Mon, 5 May 2025 at 10:54, Christian Schoenebeck
> > <qemu_oss@crudebyte.com> wrote:
> > >
> > > Add and implement functions to 9pfs test client for sending a 9p2000.L
> > > 'Tsetattr' request and receiving its 'Rsetattr' response counterpart.
> > >
> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Message-Id: <20250312152933.383967-6-groug@kaod.org>
> > > ---
> > > tests/qtest/libqos/virtio-9p-client.c | 49 +++++++++++++++++++++++++++
> > > tests/qtest/libqos/virtio-9p-client.h | 34 +++++++++++++++++++
> > > tests/qtest/virtio-9p-test.c | 1 +
> > > 3 files changed, 84 insertions(+)
> > >
> > > diff --git a/tests/qtest/libqos/virtio-9p-client.c b/tests/qtest/libqos/virtio-9p-client.c
> > > index 98b77db51d..6ab4501c6e 100644
> > > --- a/tests/qtest/libqos/virtio-9p-client.c
> > > +++ b/tests/qtest/libqos/virtio-9p-client.c
> > > @@ -557,6 +557,55 @@ void v9fs_rgetattr(P9Req *req, v9fs_attr *attr)
> > > v9fs_req_free(req);
> > > }
> > >
> > > +/*
> > > + * size[4] Tsetattr tag[2] fid[4] valid[4] mode[4] uid[4] gid[4] size[8]
> > > + * atime_sec[8] atime_nsec[8] mtime_sec[8] mtime_nsec[8]
> > > + */
> > > +TSetAttrRes v9fs_tsetattr(TSetAttrOpt opt)
> > > +{
> > > + P9Req *req;
> > > + uint32_t err;
> >
> >
> > Hi -- Coverity warns (CID 1609751) that this function
> > passes by value an argument which is a 184 byte struct.
> > Is this intentional?
>
> Hi Peter!
>
> Yes, that was intentional. It follows the same coding pattern of the 9p test
> cases to hack named function arguments into C:
>
> someFunc({ .argC = 3, .argH = "foo", .argX = 1 });
>
> That saves a lot of code and makes callers better readable, because some test
> case just needs to pass a value for argument A and C, another test might need
> to pass arguments H, X and Y, and so on.
>
> Before we had numerous function variations for the same thing, just with
> different argument permutations. Now it's only one function per purpose.
>
> > Can we instead pass a pointer to the
> > struct?
> >
> > This is only a test program and 184 bytes is not super
> > enormous, so if this would be painful to avoid we can mark
> > the coverity report as a false positive.
>
> Well, it would be possible to change this to a pointer, patch below in any
> case. Personally I would just mark this as a false positive though. It's not a
> bug and the resulting binary would probably be identical. No hard opinion
> though.
OK, I'll mark it as a false positive.
thanks
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PULL 5/9] 9pfs: Introduce ftruncate file op
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
2025-05-05 9:50 ` [PULL 2/9] 9pfs: fix FD leak and reduce latency of v9fs_reclaim_fd() Christian Schoenebeck
2025-05-05 9:50 ` [PULL 7/9] tests/9p: add 'Tsetattr' request to test client Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 4/9] 9pfs: Don't use file descriptors in core code Christian Schoenebeck
` (6 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
From: Greg Kurz <groug@kaod.org>
Add an ftruncate operation to the fs driver and use if when a fid has
a valid file descriptor. This is required to support more cases where
the client wants to do an action on an unlinked file which it still
has an open file decriptor for.
Only 9P2000.L was considered.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20250312152933.383967-4-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/file-op-9p.h | 2 ++
hw/9pfs/9p-local.c | 9 +++++++++
hw/9pfs/9p-synth.c | 8 ++++++++
hw/9pfs/9p.c | 6 +++++-
hw/9pfs/cofs.c | 18 ++++++++++++++++++
hw/9pfs/coth.h | 2 ++
6 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index b815cea44e..26ba1438c0 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -152,6 +152,8 @@ struct FileOperations {
int (*fstat)(FsContext *, int, V9fsFidOpenState *, struct stat *);
int (*rename)(FsContext *, const char *, const char *);
int (*truncate)(FsContext *, V9fsPath *, off_t);
+ int (*ftruncate)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size);
int (*fsync)(FsContext *, int, V9fsFidOpenState *, int);
int (*statfs)(FsContext *s, V9fsPath *path, struct statfs *stbuf);
ssize_t (*lgetxattr)(FsContext *, V9fsPath *,
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b16132299f..0b33da8d2a 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1042,6 +1042,14 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
return ret;
}
+static int local_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size)
+{
+ int fd = local_fid_fd(fid_type, fs);
+
+ return ftruncate(fd, size);
+}
+
static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
{
char *dirpath = g_path_get_dirname(fs_path->data);
@@ -1617,4 +1625,5 @@ FileOperations local_ops = {
.renameat = local_renameat,
.unlinkat = local_unlinkat,
.has_valid_file_handle = local_has_valid_file_handle,
+ .ftruncate = local_ftruncate,
};
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index be0492b400..3d28afc4d0 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -356,6 +356,13 @@ static int synth_truncate(FsContext *ctx, V9fsPath *path, off_t offset)
return -1;
}
+static int synth_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
static int synth_chmod(FsContext *fs_ctx, V9fsPath *path, FsCred *credp)
{
errno = EPERM;
@@ -656,4 +663,5 @@ FileOperations synth_ops = {
.renameat = synth_renameat,
.unlinkat = synth_unlinkat,
.has_valid_file_handle = synth_has_valid_file_handle,
+ .ftruncate = synth_ftruncate,
};
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4586822d24..c96f2d2d3d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1752,7 +1752,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
}
}
if (v9iattr.valid & (P9_ATTR_SIZE)) {
- err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
+ if (fid_has_valid_file_handle(pdu->s, fidp)) {
+ err = v9fs_co_ftruncate(pdu, fidp, v9iattr.size);
+ } else {
+ err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
+ }
if (err < 0) {
goto out;
}
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index 67e3ae5c5c..893466fb1a 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -184,6 +184,24 @@ int coroutine_fn v9fs_co_truncate(V9fsPDU *pdu, V9fsPath *path, off_t size)
return err;
}
+int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp, off_t size)
+{
+ int err;
+ V9fsState *s = pdu->s;
+
+ if (v9fs_request_cancelled(pdu)) {
+ return -EINTR;
+ }
+ v9fs_co_run_in_worker(
+ {
+ err = s->ops->ftruncate(&s->ctx, fidp->fid_type, &fidp->fs, size);
+ if (err < 0) {
+ err = -errno;
+ }
+ });
+ return err;
+}
+
int coroutine_fn v9fs_co_mknod(V9fsPDU *pdu, V9fsFidState *fidp,
V9fsString *name, uid_t uid, gid_t gid,
dev_t dev, mode_t mode, struct stat *stbuf)
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index 2c54249b35..62e922dc12 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -73,6 +73,8 @@ int coroutine_fn v9fs_co_chmod(V9fsPDU *, V9fsPath *, mode_t);
int coroutine_fn v9fs_co_utimensat(V9fsPDU *, V9fsPath *, struct timespec [2]);
int coroutine_fn v9fs_co_chown(V9fsPDU *, V9fsPath *, uid_t, gid_t);
int coroutine_fn v9fs_co_truncate(V9fsPDU *, V9fsPath *, off_t);
+int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
+ off_t size);
int coroutine_fn v9fs_co_llistxattr(V9fsPDU *, V9fsPath *, void *, size_t);
int coroutine_fn v9fs_co_lgetxattr(V9fsPDU *, V9fsPath *,
V9fsString *, void *, size_t);
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 4/9] 9pfs: Don't use file descriptors in core code
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (2 preceding siblings ...)
2025-05-05 9:50 ` [PULL 5/9] 9pfs: Introduce ftruncate file op Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 8/9] tests/9p: Test `Tsetattr` can truncate unlinked file Christian Schoenebeck
` (5 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
From: Greg Kurz <groug@kaod.org>
v9fs_getattr() currently peeks into V9fsFidOpenState to know if a fid
has a valid file descriptor or directory stream. Even though the fields
are accessible, this is an implementation detail of the local backend
that should not be manipulated directly by the server code.
Abstract that with a new has_valid_file_handle() backend operation.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20250312152933.383967-3-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/file-op-9p.h | 1 +
hw/9pfs/9p-local.c | 8 ++++++++
hw/9pfs/9p-synth.c | 6 ++++++
hw/9pfs/9p.c | 9 ++++++---
4 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 4997677460..b815cea44e 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -164,6 +164,7 @@ struct FileOperations {
int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char *old_name,
V9fsPath *newdir, const char *new_name);
int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int flags);
+ bool (*has_valid_file_handle)(int fid_type, V9fsFidOpenState *fs);
};
#endif
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 99b9560a52..b16132299f 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1572,6 +1572,13 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp)
return 0;
}
+static bool local_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
+{
+ return
+ (fid_type == P9_FID_FILE && fs->fd != -1) ||
+ (fid_type == P9_FID_DIR && fs->dir.stream != NULL);
+}
+
FileOperations local_ops = {
.parse_opts = local_parse_opts,
.init = local_init,
@@ -1609,4 +1616,5 @@ FileOperations local_ops = {
.name_to_path = local_name_to_path,
.renameat = local_renameat,
.unlinkat = local_unlinkat,
+ .has_valid_file_handle = local_has_valid_file_handle,
};
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 2abaf3a291..be0492b400 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -615,6 +615,11 @@ static int synth_init(FsContext *ctx, Error **errp)
return 0;
}
+static bool synth_has_valid_file_handle(int fid_type, V9fsFidOpenState *fs)
+{
+ return false;
+}
+
FileOperations synth_ops = {
.init = synth_init,
.lstat = synth_lstat,
@@ -650,4 +655,5 @@ FileOperations synth_ops = {
.name_to_path = synth_name_to_path,
.renameat = synth_renameat,
.unlinkat = synth_unlinkat,
+ .has_valid_file_handle = synth_has_valid_file_handle,
};
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 80b190ff5b..4586822d24 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1593,6 +1593,11 @@ out_nofid:
pdu_complete(pdu, err);
}
+static bool fid_has_valid_file_handle(V9fsState *s, V9fsFidState *fidp)
+{
+ return s->ops->has_valid_file_handle(fidp->fid_type, &fidp->fs);
+}
+
static void coroutine_fn v9fs_getattr(void *opaque)
{
int32_t fid;
@@ -1615,9 +1620,7 @@ static void coroutine_fn v9fs_getattr(void *opaque)
retval = -ENOENT;
goto out_nofid;
}
- if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
- (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
- {
+ if (fid_has_valid_file_handle(pdu->s, fidp)) {
retval = v9fs_co_fstat(pdu, fidp, &stbuf);
} else {
retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 8/9] tests/9p: Test `Tsetattr` can truncate unlinked file
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (3 preceding siblings ...)
2025-05-05 9:50 ` [PULL 4/9] 9pfs: Don't use file descriptors in core code Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 6/9] 9pfs: Introduce futimens file op Christian Schoenebeck
` (4 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
From: Greg Kurz <groug@kaod.org>
Enhance the `use-after-unlink` test with a new check for the
case where the client wants to alter the size of an unlinked
file for which it still has an active fid.
Suggested-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20250312152933.383967-7-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
tests/qtest/virtio-9p-test.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index f515a9bb15..ac38ccf595 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -736,6 +736,20 @@ static void fs_use_after_unlink(void *obj, void *data,
.data = buf
}).count;
g_assert_cmpint(count, ==, write_count);
+
+ /* truncate file to (arbitrarily chosen) size 2001 */
+ tsetattr({
+ .client = v9p, .fid = fid_file, .attr = (v9fs_attr) {
+ .valid = P9_SETATTR_SIZE,
+ .size = 2001
+ }
+ });
+ /* truncate apparently succeeded, let's double-check the size */
+ tgetattr({
+ .client = v9p, .fid = fid_file, .request_mask = P9_GETATTR_BASIC,
+ .rgetattr.attr = &attr
+ });
+ g_assert_cmpint(attr.size, ==, 2001);
}
static void cleanup_9p_local_driver(void *data)
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 6/9] 9pfs: Introduce futimens file op
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (4 preceding siblings ...)
2025-05-05 9:50 ` [PULL 8/9] tests/9p: Test `Tsetattr` can truncate unlinked file Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 9/9] 9pfs: fix 'total_open_fd' decrementation Christian Schoenebeck
` (3 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
From: Greg Kurz <groug@kaod.org>
Add an futimens operation to the fs driver and use if when a fid has
a valid file descriptor. This is required to support more cases where
the client wants to do an action on an unlinked file which it still
has an open file decriptor for.
Only 9P2000.L was considered.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20250312152933.383967-5-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
fsdev/file-op-9p.h | 2 ++
hw/9pfs/9p-local.c | 9 +++++++++
hw/9pfs/9p-synth.c | 8 ++++++++
hw/9pfs/9p-util.h | 1 +
hw/9pfs/9p.c | 6 +++++-
hw/9pfs/cofs.c | 19 +++++++++++++++++++
hw/9pfs/coth.h | 2 ++
7 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 26ba1438c0..b9dae8c84c 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -129,6 +129,8 @@ struct FileOperations {
int (*chown)(FsContext *, V9fsPath *, FsCred *);
int (*mknod)(FsContext *, V9fsPath *, const char *, FsCred *);
int (*utimensat)(FsContext *, V9fsPath *, const struct timespec *);
+ int (*futimens)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *times);
int (*remove)(FsContext *, const char *);
int (*symlink)(FsContext *, const char *, V9fsPath *,
const char *, FsCred *);
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 0b33da8d2a..31e216227c 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1100,6 +1100,14 @@ out:
return ret;
}
+static int local_futimens(FsContext *s, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *times)
+{
+ int fd = local_fid_fd(fid_type, fs);
+
+ return qemu_futimens(fd, times);
+}
+
static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
int flags)
{
@@ -1626,4 +1634,5 @@ FileOperations local_ops = {
.unlinkat = local_unlinkat,
.has_valid_file_handle = local_has_valid_file_handle,
.ftruncate = local_ftruncate,
+ .futimens = local_futimens,
};
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 3d28afc4d0..9cd1884224 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -424,6 +424,13 @@ static int synth_utimensat(FsContext *fs_ctx, V9fsPath *path,
return 0;
}
+static int synth_futimens(FsContext *fs_ctx, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *buf)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
static int synth_remove(FsContext *ctx, const char *path)
{
errno = EPERM;
@@ -664,4 +671,5 @@ FileOperations synth_ops = {
.unlinkat = synth_unlinkat,
.has_valid_file_handle = synth_has_valid_file_handle,
.ftruncate = synth_ftruncate,
+ .futimens = synth_futimens,
};
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 7bc4ec8e85..a1924fe3f0 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -103,6 +103,7 @@ static inline int errno_to_dotl(int err) {
#define qemu_renameat renameat
#define qemu_utimensat utimensat
#define qemu_unlinkat unlinkat
+#define qemu_futimens futimens
static inline void close_preserve_errno(int fd)
{
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c96f2d2d3d..b22df3aa2b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1727,7 +1727,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
} else {
times[1].tv_nsec = UTIME_OMIT;
}
- err = v9fs_co_utimensat(pdu, &fidp->path, times);
+ if (fid_has_valid_file_handle(pdu->s, fidp)) {
+ err = v9fs_co_futimens(pdu, fidp, times);
+ } else {
+ err = v9fs_co_utimensat(pdu, &fidp->path, times);
+ }
if (err < 0) {
goto out;
}
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index 893466fb1a..12fa8c9fe9 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -139,6 +139,25 @@ int coroutine_fn v9fs_co_utimensat(V9fsPDU *pdu, V9fsPath *path,
return err;
}
+int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct timespec times[2])
+{
+ int err;
+ V9fsState *s = pdu->s;
+
+ if (v9fs_request_cancelled(pdu)) {
+ return -EINTR;
+ }
+ v9fs_co_run_in_worker(
+ {
+ err = s->ops->futimens(&s->ctx, fidp->fid_type, &fidp->fs, times);
+ if (err < 0) {
+ err = -errno;
+ }
+ });
+ return err;
+}
+
int coroutine_fn v9fs_co_chown(V9fsPDU *pdu, V9fsPath *path, uid_t uid,
gid_t gid)
{
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index 62e922dc12..7906fa7782 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -71,6 +71,8 @@ int coroutine_fn v9fs_co_statfs(V9fsPDU *, V9fsPath *, struct statfs *);
int coroutine_fn v9fs_co_lstat(V9fsPDU *, V9fsPath *, struct stat *);
int coroutine_fn v9fs_co_chmod(V9fsPDU *, V9fsPath *, mode_t);
int coroutine_fn v9fs_co_utimensat(V9fsPDU *, V9fsPath *, struct timespec [2]);
+int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct timespec times[2]);
int coroutine_fn v9fs_co_chown(V9fsPDU *, V9fsPath *, uid_t, gid_t);
int coroutine_fn v9fs_co_truncate(V9fsPDU *, V9fsPath *, off_t);
int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 9/9] 9pfs: fix 'total_open_fd' decrementation
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (5 preceding siblings ...)
2025-05-05 9:50 ` [PULL 6/9] 9pfs: Introduce futimens file op Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 3/9] 9pfs: local : Introduce local_fid_fd() helper Christian Schoenebeck
` (2 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
According to 'man 2 close' errors returned by close() should only be used
for either diagnostic purposes or for catching data loss due to a previous
write error, as an error result of close() usually indicates a deferred
error of a previous write operation.
Therefore not decrementing 'total_open_fd' on a close() error is wrong
and would yield in a higher open file descriptor count than actually the
case, leading to 9p server reclaiming open file descriptors too soon.
Based-on: <20250312152933.383967-7-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <E1tvEyJ-004dMa-So@kylie.crudebyte.com>
---
hw/9pfs/9p.c | 10 +++++++++-
hw/9pfs/codir.c | 7 ++++++-
hw/9pfs/cofile.c | 7 ++++++-
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b22df3aa2b..8b001b9112 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -510,7 +510,15 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
err = (f->fid_type == P9_FID_DIR) ?
s->ops->closedir(&s->ctx, &f->fs_reclaim) :
s->ops->close(&s->ctx, &f->fs_reclaim);
- if (!err) {
+
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /*
+ * unexpected case as FIDs were picked above by having a valid
+ * file descriptor
+ */
+ error_report("9pfs: v9fs_reclaim_fd() WARNING: close() failed with EBADF");
+ } else {
/* total_open_fd must only be mutated on main thread */
nclosed++;
}
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 2068a4779d..bce7dd96e9 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -20,6 +20,7 @@
#include "fsdev/qemu-fsdev.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
#include "coth.h"
#include "9p-xattr.h"
#include "9p-util.h"
@@ -353,7 +354,11 @@ int coroutine_fn v9fs_co_closedir(V9fsPDU *pdu, V9fsFidOpenState *fs)
err = -errno;
}
});
- if (!err) {
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /* unexpected case as we should have checked for a valid file handle */
+ error_report("9pfs: WARNING: v9fs_co_closedir() failed with EBADF");
+ } else {
total_open_fd--;
}
return err;
diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 71174c3e4a..6e775c8e41 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -20,6 +20,7 @@
#include "fsdev/qemu-fsdev.h"
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
#include "coth.h"
int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
@@ -197,7 +198,11 @@ int coroutine_fn v9fs_co_close(V9fsPDU *pdu, V9fsFidOpenState *fs)
err = -errno;
}
});
- if (!err) {
+ /* 'man 2 close' suggests to ignore close() errors except of EBADF */
+ if (unlikely(err && errno == EBADF)) {
+ /* unexpected case as we should have checked for a valid file handle */
+ error_report("9pfs: WARNING: v9fs_co_close() failed with EBADF");
+ } else {
total_open_fd--;
}
return err;
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 3/9] 9pfs: local : Introduce local_fid_fd() helper
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (6 preceding siblings ...)
2025-05-05 9:50 ` [PULL 9/9] 9pfs: fix 'total_open_fd' decrementation Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-05 9:50 ` [PULL 1/9] 9pfs: fix concurrent v9fs_reclaim_fd() calls Christian Schoenebeck
2025-05-06 13:58 ` [PULL 0/9] 9p queue 2025-05-05 Stefan Hajnoczi
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
From: Greg Kurz <groug@kaod.org>
Factor out duplicated code to a single helper. More users to come.
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20250312152933.383967-2-groug@kaod.org>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
hw/9pfs/9p-local.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 928523afcc..99b9560a52 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -766,16 +766,19 @@ out:
return err;
}
+static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
+{
+ if (fid_type == P9_FID_DIR) {
+ return dirfd(fs->dir.stream);
+ } else {
+ return fs->fd;
+ }
+}
+
static int local_fstat(FsContext *fs_ctx, int fid_type,
V9fsFidOpenState *fs, struct stat *stbuf)
{
- int err, fd;
-
- if (fid_type == P9_FID_DIR) {
- fd = dirfd(fs->dir.stream);
- } else {
- fd = fs->fd;
- }
+ int err, fd = local_fid_fd(fid_type, fs);
err = fstat(fd, stbuf);
if (err) {
@@ -1167,13 +1170,7 @@ out:
static int local_fsync(FsContext *ctx, int fid_type,
V9fsFidOpenState *fs, int datasync)
{
- int fd;
-
- if (fid_type == P9_FID_DIR) {
- fd = dirfd(fs->dir.stream);
- } else {
- fd = fs->fd;
- }
+ int fd = local_fid_fd(fid_type, fs);
if (datasync) {
return qemu_fdatasync(fd);
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 1/9] 9pfs: fix concurrent v9fs_reclaim_fd() calls
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (7 preceding siblings ...)
2025-05-05 9:50 ` [PULL 3/9] 9pfs: local : Introduce local_fid_fd() helper Christian Schoenebeck
@ 2025-05-05 9:50 ` Christian Schoenebeck
2025-05-06 13:58 ` [PULL 0/9] 9p queue 2025-05-05 Stefan Hajnoczi
9 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-05-05 9:50 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: Greg Kurz
Even though this function is serialized to be always called from main
thread, v9fs_reclaim_fd() is dispatching the coroutine to a worker thread
in between via its v9fs_co_*() calls, hence leading to the situation where
v9fs_reclaim_fd() is effectively executed multiple times simultaniously,
which renders its LRU algorithm useless and causes high latency.
Fix this by adding a simple boolean variable to ensure this function is
only called once at a time. No synchronization needed for this boolean
variable as this function is only entered and returned on main thread.
Fixes: 7a46274529c ('hw/9pfs: Add file descriptor reclaim support')
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Message-Id: <5c622067efd66dd4ee5eca740dcf263f41db20b2.1741339452.git.qemu_oss@crudebyte.com>
---
hw/9pfs/9p.c | 10 ++++++++++
hw/9pfs/9p.h | 1 +
2 files changed, 11 insertions(+)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7cad2bce62..4f9c2dde9c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -435,6 +435,12 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
GHashTableIter iter;
gpointer fid;
+ /* prevent multiple coroutines running this function simultaniously */
+ if (s->reclaiming) {
+ return;
+ }
+ s->reclaiming = true;
+
g_hash_table_iter_init(&iter, s->fids);
QSLIST_HEAD(, V9fsFidState) reclaim_list =
@@ -510,6 +516,8 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
*/
put_fid(pdu, f);
}
+
+ s->reclaiming = false;
}
/*
@@ -4324,6 +4332,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
s->ctx.fst = &fse->fst;
fsdev_throttle_init(s->ctx.fst);
+ s->reclaiming = false;
+
rc = 0;
out:
if (rc) {
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5e041e1f60..259ad32ed1 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -362,6 +362,7 @@ struct V9fsState {
uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
uint16_t qp_affix_next;
uint64_t qp_fullpath_next;
+ bool reclaiming;
};
/* 9p2000.L open flags */
--
2.30.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 0/9] 9p queue 2025-05-05
2025-05-05 9:50 [PULL 0/9] 9p queue 2025-05-05 Christian Schoenebeck
` (8 preceding siblings ...)
2025-05-05 9:50 ` [PULL 1/9] 9pfs: fix concurrent v9fs_reclaim_fd() calls Christian Schoenebeck
@ 2025-05-06 13:58 ` Stefan Hajnoczi
9 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2025-05-06 13:58 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel, Peter Maydell, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 116 bytes --]
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread