Hi, Xiang, On 1/22/24 3:42 PM, Gao Xiang wrote: > Hi Jingbo, > > On 2024/1/22 15:12, Jingbo Xu wrote: >> So far the fscache mode supports uncompressed data only, and the data >> read from fscache is put directly into the target page cache. As the >> support for compressed data in fscache mode is going to be introduced, >> refactor the interface of reading fscache so that the following >> compressed part could make the raw data read from fscache be directed to >> the target buffer it wants, decompress the raw data, and finally fill >> the page cache with the decompressed data. >> >> As the first step, a new structure, i.e. erofs_fscache_io (cio), is > > I'd suggest just using io instead of cio here. > > .. i.e. erofs_fscache_io (io) ...
Thanks for the review, I will update the patch regarding all these comments. > >> introduced to describe a generic read request from the fscache, while >> the caller can specify the target buffer it wants in the iov_iter >> structure (cio->iter). Besides, the caller can also specify its > > .. structure (io->iter) ... > >> completion callback and private data through cio, which will be called >> to make further handling, e.g. unlocking the page cache for uncompressed >> data or decompressing the read raw data, when the read request from the >> fscache completes. Now erofs_fscache_read_io_async() serves as a >> generic interface for reading raw data from fscache for both compressed >> and uncompressed data. >> >> The erofs_fscache_request structure is kept to describe a request to >> fill the page cache in the specified range. >> >> Signed-off-by: Jingbo Xu <[email protected]> >> --- >> fs/erofs/fscache.c | 219 ++++++++++++++++++++++++--------------------- >> 1 file changed, 118 insertions(+), 101 deletions(-) >> >> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c >> index bc12030393b2..10709f20bef5 100644 >> --- a/fs/erofs/fscache.c >> +++ b/fs/erofs/fscache.c >> @@ -13,8 +13,6 @@ static LIST_HEAD(erofs_domain_cookies_list); >> static struct vfsmount *erofs_pseudo_mnt; >> struct erofs_fscache_request { >> - struct erofs_fscache_request *primary; >> - struct netfs_cache_resources cache_resources; >> struct address_space *mapping; /* The mapping being >> accessed */ >> loff_t start; /* Start position */ >> size_t len; /* Length of the request */ >> @@ -23,42 +21,13 @@ struct erofs_fscache_request { >> refcount_t ref; >> }; >> -static struct erofs_fscache_request *erofs_fscache_req_alloc(struct >> address_space *mapping, >> - loff_t start, size_t len) >> -{ >> - struct erofs_fscache_request *req; >> - >> - req = kzalloc(sizeof(struct erofs_fscache_request), GFP_KERNEL); >> - if (!req) >> - return ERR_PTR(-ENOMEM); >> - >> - req->mapping = mapping; >> - req->start = start; >> - req->len = len; >> - refcount_set(&req->ref, 1); >> - >> - return req; >> -} >> - >> -static struct erofs_fscache_request *erofs_fscache_req_chain(struct >> erofs_fscache_request *primary, >> - size_t len) >> -{ >> - struct erofs_fscache_request *req; >> - >> - /* use primary request for the first submission */ >> - if (!primary->submitted) { >> - refcount_inc(&primary->ref); >> - return primary; >> - } >> - >> - req = erofs_fscache_req_alloc(primary->mapping, >> - primary->start + primary->submitted, len); >> - if (!IS_ERR(req)) { >> - req->primary = primary; >> - refcount_inc(&primary->ref); >> - } >> - return req; >> -} >> +struct erofs_fscache_io { >> + struct netfs_cache_resources cache_resources; > > struct netfs_cache_resources cres; > >> + struct iov_iter iter; >> + netfs_io_terminated_t end_io; >> + void *private; >> + refcount_t ref; >> +}; >> static void erofs_fscache_req_complete(struct >> erofs_fscache_request *req) >> { >> @@ -83,82 +52,116 @@ static void erofs_fscache_req_complete(struct >> erofs_fscache_request *req) >> static void erofs_fscache_req_put(struct erofs_fscache_request *req) >> { >> if (refcount_dec_and_test(&req->ref)) { >> - if (req->cache_resources.ops) >> - >> req->cache_resources.ops->end_operation(&req->cache_resources); >> - if (!req->primary) >> - erofs_fscache_req_complete(req); >> - else >> - erofs_fscache_req_put(req->primary); >> + erofs_fscache_req_complete(req); >> kfree(req); >> } >> } >> -static void erofs_fscache_subreq_complete(void *priv, >> +static struct erofs_fscache_request *erofs_fscache_req_alloc(struct >> address_space *mapping, >> + loff_t start, size_t len) >> +{ >> + struct erofs_fscache_request *req; >> + >> + req = kzalloc(sizeof(*req), GFP_KERNEL); >> + if (req) { >> + req->mapping = mapping; >> + req->start = start; >> + req->len = len; >> + refcount_set(&req->ref, 1); >> + } >> + return req; > > The following part may be better? to save an indentation: > > req = kzalloc(sizeof(*req), GFP_KERNEL); > if (!req) > return NULL; > req->mapping = mapping; > req->start = start; > req->len = len; > refcount_set(&req->ref, 1); > return req; > >> +} >> + >> +static bool erofs_fscache_io_put(struct erofs_fscache_io *cio) >> +{ >> + if (refcount_dec_and_test(&cio->ref)) { >> + if (cio->cache_resources.ops) >> + >> cio->cache_resources.ops->end_operation(&cio->cache_resources); >> + kfree(cio); >> + return true; >> + } >> + return false; > > > if (!refcount_dec_and_test(&io->ref)) > return false; > if (io->cres.ops) > io->cres.ops->end_operation(&io->cres); > kfree(io); > return true; > >> +} >> + >> +static void erofs_fscache_req_io_put(struct erofs_fscache_io *cio) > > cio -> io > >> +{ >> + struct erofs_fscache_request *req = cio->private; >> + >> + if (erofs_fscache_io_put(cio)) >> + erofs_fscache_req_put(req); >> +} >> + >> +static void erofs_fscache_req_end_io(void *priv, >> ssize_t transferred_or_error, bool was_async) >> { >> - struct erofs_fscache_request *req = priv; >> + struct erofs_fscache_io *cio = priv; >> + struct erofs_fscache_request *req = cio->private; >> + >> + if (IS_ERR(transferred_or_error)) >> + req->error = transferred_or_error; >> + erofs_fscache_req_io_put(cio); >> +} >> + >> +static struct erofs_fscache_io *erofs_fscache_req_io_alloc(struct >> erofs_fscache_request *req) >> +{ >> + struct erofs_fscache_io *cio; >> - if (IS_ERR_VALUE(transferred_or_error)) { >> - if (req->primary) >> - req->primary->error = transferred_or_error; >> - else >> - req->error = transferred_or_error; >> + cio = kzalloc(sizeof(*cio), GFP_KERNEL); >> + if (cio) { >> + cio->end_io = erofs_fscache_req_end_io; >> + cio->private = req; >> + refcount_inc(&req->ref); >> + refcount_set(&cio->ref, 1); >> } >> - erofs_fscache_req_put(req); >> + return cio; > > > io = kzalloc(sizeof(*io), GFP_KERNEL); > if (!io) > return NULL; > io->end_io = erofs_fscache_req_end_io; > io->private = req; > refcount_inc(&req->ref); > refcount_set(&io->ref, 1); > return io; > > Thanks, > Gao Xiang -- Thanks, Jingbo
