Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 134 additions & 7 deletions fs/fuse/dev_uring.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,67 @@ static void fuse_uring_flush_queue_bg(struct fuse_ring_queue *queue)
}
}

/*
* With the writeback cache, foreground requests may hold at most half of the
* entries. They might wait in userspace on DLM revokes, which wait on page
* invalidation, which waits on writeback - if they held all entries,
* background writeback could not be sent and none of them would complete.
*/
#define FUSE_URING_MIN_FOREGROUND 8U

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used any where, so this can probably come out unless there was some use case that was forgotten

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Allison. Here I need to refine it for the calculation... but team is rushing to provide the testing, thus quickly merging the PR before I complete my another working commit... so I have to create another PR for my next commit for the io-uring change...

#define FUSE_URING_MIN_BACKGROUND 2U

static unsigned int fuse_uring_max_background(struct fuse_ring_queue *queue)
{
return max(queue->nr_ents / 4, FUSE_URING_MIN_BACKGROUND);
}

static unsigned int fuse_uring_max_foreground(struct fuse_ring_queue *queue)
{
unsigned int bg = fuse_uring_max_background(queue);

if (queue->nr_ents <= bg) {
pr_warn_once("fuse-io-uring configuration value is too small, could cause hung issue\n");
return queue->nr_ents;
}

return queue->nr_ents - bg;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was FUSE_URING_MIN_FOREGROUND maybe supposed to go here? This works out to nr_ents - max(nr_ents/4, 2). I think (max(nr_ents - bg, min(FUSE_URING_MIN_FOREGROUND, nr_ents))) makes more sense?

That would match the commit message "Foreground requests may use the rest, but at least 8" and "if a queue has fewer than 10 entries ... the foreground minimum wins."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Here I need to revise it too... Anyway, I will have another commit to contains this. Thanks.

}

static bool fuse_uring_req_fg_limited(struct fuse_req *req)
{
return req->fm->fc->writeback_cache &&
!test_bit(FR_BACKGROUND, &req->flags) &&
!(req->args && req->args->no_fg_limit);
}

static void fuse_uring_flush_queue_fg(struct fuse_ring_queue *queue)
{
lockdep_assert_held(&queue->lock);

while (queue->active_foreground < fuse_uring_max_foreground(queue) &&
!list_empty(&queue->fuse_req_fg_queue)) {
struct fuse_req *req;

req = list_first_entry(&queue->fuse_req_fg_queue,
struct fuse_req, list);
set_bit(FR_URING_FG, &req->flags);
queue->active_foreground++;

list_move_tail(&req->list, &queue->fuse_req_queue);
}
}

static void fuse_uring_end_foreground(struct fuse_ring_queue *queue,
struct fuse_req *req)
{
lockdep_assert_held(&queue->lock);

if (test_and_clear_bit(FR_URING_FG, &req->flags)) {
queue->active_foreground--;
fuse_uring_flush_queue_fg(queue);
}
}

static void __fuse_uring_req_end(struct fuse_ring_ent *ent,
struct fuse_req *req, int error)
{
Expand All @@ -112,6 +173,7 @@ static void __fuse_uring_req_end(struct fuse_ring_ent *ent,
ent->fuse_req = NULL;
queue->nr_reqs--;
list_del_init(&req->list);
fuse_uring_end_foreground(queue, req);
if (test_bit(FR_BACKGROUND, &req->flags)) {
queue->active_background--;
spin_lock(&fc->bg_lock);
Expand Down Expand Up @@ -171,9 +233,15 @@ static void fuse_uring_abort_end_queue_requests(struct fuse_ring_queue *queue)
LIST_HEAD(req_list);

spin_lock(&queue->lock);
list_for_each_entry(req, &queue->fuse_req_queue, list)
list_for_each_entry(req, &queue->fuse_req_queue, list) {
clear_bit(FR_PENDING, &req->flags);
if (test_and_clear_bit(FR_URING_FG, &req->flags))
queue->active_foreground--;
}
list_for_each_entry(req, &queue->fuse_req_fg_queue, list)
clear_bit(FR_PENDING, &req->flags);
list_splice_init(&queue->fuse_req_queue, &req_list);
list_splice_init(&queue->fuse_req_fg_queue, &req_list);
queue->nr_reqs = 0;
spin_unlock(&queue->lock);

Expand Down Expand Up @@ -237,6 +305,7 @@ bool fuse_uring_request_expired(struct fuse_conn *fc)
spin_lock(&queue->lock);
if (fuse_request_expired(fc, &queue->fuse_req_queue) ||
fuse_request_expired(fc, &queue->fuse_req_bg_queue) ||
fuse_request_expired(fc, &queue->fuse_req_fg_queue) ||
ent_list_request_expired(fc, &queue->ent_w_req_queue) ||
ent_list_request_expired(fc, &queue->ent_in_userspace)) {
spin_unlock(&queue->lock);
Expand Down Expand Up @@ -461,6 +530,7 @@ static struct fuse_ring_queue *fuse_uring_create_queue(struct fuse_ring *ring,
INIT_LIST_HEAD(&queue->ent_in_userspace);
INIT_LIST_HEAD(&queue->fuse_req_queue);
INIT_LIST_HEAD(&queue->fuse_req_bg_queue);
INIT_LIST_HEAD(&queue->fuse_req_fg_queue);
INIT_LIST_HEAD(&queue->ent_released);

queue->fpq.processing = pq;
Expand Down Expand Up @@ -524,7 +594,10 @@ static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent, int issue_flags
if (req) {
/* remove entry from queue->fpq->processing */
list_del_init(&req->list);
if (test_and_clear_bit(FR_URING_FG, &req->flags))
queue->active_foreground--;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a fuse_uring_flush_queue_fg for decrementing the foreground?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure for now, will check it later.

}
queue->nr_ents--;

/*
* The entry must not be freed immediately, due to access of direct
Expand Down Expand Up @@ -1138,6 +1211,8 @@ static int fuse_ring_ent_set_commit(struct fuse_ring_ent *ent)
return 0;
}

static void fuse_uring_dispatch_ent(struct fuse_ring_ent *ent, bool bg);

/* FUSE_URING_CMD_COMMIT_AND_FETCH handler */
static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
struct fuse_conn *fc)
Expand All @@ -1151,6 +1226,8 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
unsigned int qid = READ_ONCE(cmd_req->qid);
struct fuse_pqueue *fpq;
struct fuse_req *req;
struct fuse_ring_ent *avail;
struct fuse_req *next = NULL;

err = -ENOTCONN;
if (!ring)
Expand Down Expand Up @@ -1189,7 +1266,32 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
if (err != 0) {
pr_info_ratelimited("qid=%d commit_id %llu state %d",
queue->qid, commit_id, ent->state);
queue->nr_reqs--;
fuse_uring_end_foreground(queue, req);
if (test_bit(FR_BACKGROUND, &req->flags)) {
queue->active_background--;
spin_lock(&fc->bg_lock);
fuse_uring_flush_queue_bg(queue);
spin_unlock(&fc->bg_lock);
}

/*
* The freed foreground or background slot might have moved a
* queued request to fuse_req_queue. This entry is not usable,
* so hand the request to another available entry.
*/
avail = list_first_entry_or_null(&queue->ent_avail_queue,
struct fuse_ring_ent, list);
if (avail)
next = list_first_entry_or_null(&queue->fuse_req_queue,
struct fuse_req, list);
if (next)
fuse_uring_add_req_to_ring_ent(avail, next);
spin_unlock(&queue->lock);

if (next)
fuse_uring_dispatch_ent(avail, true);

req->out.h.error = err;
clear_bit(FR_SENT, &req->flags);
fuse_request_end(req);
Expand Down Expand Up @@ -1434,6 +1536,8 @@ static int fuse_uring_register(struct io_uring_cmd *cmd,

spin_lock(&queue->lock);
ent->cmd = cmd;
queue->nr_ents++;
fuse_uring_flush_queue_fg(queue);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since the queue is still empty here, we're going to get a warning every time the module loads. I wonder if we need the flush at this point since we're still registering?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! Let me think about this, and address in the working commit if needed. Thanks, and please help to continue review the codes, then I can try to address all these in one commit:-)

spin_unlock(&queue->lock);

/* Marks the ring entry as ready */
Expand Down Expand Up @@ -1670,6 +1774,7 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req)
struct fuse_ring *ring = fc->ring;
struct fuse_ring_queue *queue;
struct fuse_ring_ent *ent = NULL;
struct fuse_req *next;
int err;

err = -EINVAL;
Expand All @@ -1684,18 +1789,25 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req)

set_bit(FR_URING, &req->flags);
req->ring_queue = queue;
ent = list_first_entry_or_null(&queue->ent_avail_queue,
struct fuse_ring_ent, list);
queue->nr_reqs++;

if (ent)
fuse_uring_add_req_to_ring_ent(ent, req);
else
if (fuse_uring_req_fg_limited(req)) {
list_add_tail(&req->list, &queue->fuse_req_fg_queue);
fuse_uring_flush_queue_fg(queue);
} else {
list_add_tail(&req->list, &queue->fuse_req_queue);
}

ent = list_first_entry_or_null(&queue->ent_avail_queue,
struct fuse_ring_ent, list);
next = ent ? list_first_entry_or_null(&queue->fuse_req_queue,
struct fuse_req, list) : NULL;
if (next)
fuse_uring_add_req_to_ring_ent(ent, next);

spin_unlock(&queue->lock);

if (ent)
if (next)
fuse_uring_dispatch_ent(ent, false);

return;
Expand Down Expand Up @@ -1762,12 +1874,27 @@ bool fuse_uring_remove_pending_req(struct fuse_req *req)
{
struct fuse_ring_queue *queue = req->ring_queue;
bool removed = fuse_remove_pending_req(req, &queue->lock);
struct fuse_ring_ent *ent;
struct fuse_req *next = NULL;

if (removed) {
/* Update counters after successful removal */
spin_lock(&queue->lock);
queue->nr_reqs--;
fuse_uring_end_foreground(queue, req);

/* the freed foreground slot might let a queued request run */
ent = list_first_entry_or_null(&queue->ent_avail_queue,
struct fuse_ring_ent, list);
if (ent)
next = list_first_entry_or_null(&queue->fuse_req_queue,
struct fuse_req, list);
if (next)
fuse_uring_add_req_to_ring_ent(ent, next);
spin_unlock(&queue->lock);

if (next)
fuse_uring_dispatch_ent(ent, true);
}

return removed;
Expand Down
13 changes: 13 additions & 0 deletions fs/fuse/dev_uring_i.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,19 @@ struct fuse_ring_queue {
/* background fuse requests */
struct list_head fuse_req_bg_queue;

/* foreground fuse requests waiting for the foreground limit */
struct list_head fuse_req_fg_queue;

/* number of registered ring entries */
unsigned int nr_ents;

/*
* foreground requests on fuse_req_queue or in userspace, limited to
* the entries not reserved for background requests with writeback cache;
* see fuse_uring_max_foreground()
*/
unsigned int active_foreground;

/* number of requests queued or in userspace */
unsigned int nr_reqs;

Expand Down
2 changes: 2 additions & 0 deletions fs/fuse/dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -2100,6 +2100,8 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
if (fm->fc->setattr_writeback)
inarg.valid |= FATTR_WRITEBACK;
fuse_setattr_fill(fm->fc, &args, inode, &inarg, &outarg);
/* might be called from writeback, like a background request */
args.no_fg_limit = true;

return fuse_simple_request(fm, &args);
}
Expand Down
3 changes: 3 additions & 0 deletions fs/fuse/file.c
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,7 @@ static int fuse_do_readfolio(struct file *file, struct folio *folio,
struct fuse_io_args ia = {
.ap.args.page_zeroing = true,
.ap.args.out_pages = true,
.ap.args.no_fg_limit = true,
.ap.num_folios = 1,
.ap.folios = &folio,
.ap.descs = &desc,
Expand Down Expand Up @@ -1164,6 +1165,7 @@ static void fuse_send_readpages(struct fuse_io_args *ia, struct file *file,
ap->args.out_pages = true;
ap->args.page_zeroing = true;
ap->args.page_replace = true;
ap->args.no_fg_limit = true;

/* Don't overflow end offset */
if (pos + (count - 1) == LLONG_MAX) {
Expand Down Expand Up @@ -1593,6 +1595,7 @@ static ssize_t fuse_send_write_pages(struct fuse_io_args *ia,
if (fm->fc->handle_killpriv_v2 && !capable(CAP_FSETID))
ia->write.in.write_flags |= FUSE_WRITE_KILL_SUIDGID;

ap->args.no_fg_limit = true;
err = fuse_simple_request(fm, &ap->args);
if (!err && ia->write.out.size > count)
err = -EIO;
Expand Down
3 changes: 3 additions & 0 deletions fs/fuse/fuse_i.h
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ struct fuse_args {
bool is_ext:1;
bool is_pinned:1;
bool invalidate_vmap:1;
bool no_fg_limit:1;
struct fuse_in_arg in_args[4];
struct fuse_arg out_args[2];
void (*end)(struct fuse_mount *fm, struct fuse_args *args, int error);
Expand Down Expand Up @@ -520,6 +521,7 @@ struct fuse_io_priv {
* FR_PRIVATE: request is on private list
* FR_ASYNC: request is asynchronous
* FR_URING: request is handled through fuse-io-uring
* FR_URING_FG: request is counted in the io-uring queue foreground limit
*/
enum fuse_req_flag {
FR_ISREPLY,
Expand All @@ -536,6 +538,7 @@ enum fuse_req_flag {
FR_PRIVATE,
FR_ASYNC,
FR_URING,
FR_URING_FG,
};

/**
Expand Down
Loading