On Tue, Aug 13, 2024 at 10:04 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 8/8/24 21:01, Joanne Koong wrote:
>
> > @@ -1951,9 +2115,10 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> > goto copy_finish;
> > }
> >
> > + __fuse_get_request(req);
> > +
> > /* Is it an interrupt reply ID? */
> > if (oh.unique & FUSE_INT_REQ_BIT) {
> > - __fuse_get_request(req);
> > spin_unlock(&fpq->lock);
> >
> > err = 0;
> > @@ -1969,6 +2134,13 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> > goto copy_finish;
> > }
> >
> > + if (test_and_set_bit(FR_FINISHING, &req->flags)) {
> > + /* timeout handler is already finishing the request */
> > + spin_unlock(&fpq->lock);
> > + fuse_put_request(req);
> > + goto copy_finish;
> > + }
> > +
>
> It should be safe already with the FR_FINISHING flag and
> timer_delete_sync, but maybe we could unset req->fpq here to make that
> easier to read and to be double sure?
Sure, I can add this into v4. I'll add a comment as well explaining
that it's not necessary but is here as a safeguard to ensure that the
timeout handler is a no-op.
>
> > clear_bit(FR_SENT, &req->flags);
> > list_move(&req->list, &fpq->io);
> > req->out.h = oh;
> > @@ -1995,6 +2167,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> > spin_unlock(&fpq->lock);
> >
> > fuse_request_end(req);
> > + fuse_put_request(req);
> > out:
> > return err ? err : nbytes;
> >
> > @@ -2260,13 +2433,21 @@ int fuse_dev_release(struct inode *inode, struct file *file)
> > if (fud) {
> > struct fuse_conn *fc = fud->fc;
> > struct fuse_pqueue *fpq = &fud->pq;
> > + struct fuse_req *req;
> > LIST_HEAD(to_end);
> > unsigned int i;
> >
> > spin_lock(&fpq->lock);
> > WARN_ON(!list_empty(&fpq->io));
> > - for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
> > + for (i = 0; i < FUSE_PQ_HASH_SIZE; i++) {
> > + /*
> > + * Set the req error here so that the timeout
> > + * handler knows it's being released
> > + */
> > + list_for_each_entry(req, &fpq->processing[i], list)
> > + req->out.h.error = -ECONNABORTED;
> > list_splice_init(&fpq->processing[i], &to_end);
> > + }
> > spin_unlock(&fpq->lock);
> >
> > end_requests(&to_end);
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index f23919610313..2b616c5977b4 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -375,6 +375,8 @@ struct fuse_io_priv {
> > * FR_FINISHED: request is finished
> > * FR_PRIVATE: request is on private list
> > * FR_ASYNC: request is asynchronous
> > + * FR_FINISHING: request is being finished, by either the timeout handler
> > + * or the reply handler
> > */
> > enum fuse_req_flag {
> > FR_ISREPLY,
> > @@ -389,6 +391,7 @@ enum fuse_req_flag {
> > FR_FINISHED,
> > FR_PRIVATE,
> > FR_ASYNC,
> > + FR_FINISHING,
> > };
> >
> > /**
> > @@ -435,6 +438,12 @@ struct fuse_req {
> >
> > /** fuse_mount this request belongs to */
> > struct fuse_mount *fm;
> > +
> > + /** page queue this request has been added to */
> > + struct fuse_pqueue *fpq;
>
> Processing queue?
You're right, this should be "processing queue", I'll make this change in v4.
Thanks,
Joanne
>
>
> Thanks,
> Bernd
