| Message ID | 20221104072637.72375-2-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fscache,cachefiles: add prepare_ondemand_read() interface | expand |
On Fri, 2022-11-04 at 15:26 +0800, Jingbo Xu wrote: > Add prepare_ondemand_read() callback dedicated for the on-demand read > scenario, so that callers from this scenario can be decoupled from > netfs_io_subrequest. > > To reuse the hole detecting logic as mush as possible, both the > implementation of prepare_read() and prepare_ondemand_read() inside > Cachefiles call a common routine. > > In the near future, prepare_read() will get enhanced and more > information will be needed and then returned to callers. Thus > netfs_io_subrequest is a reasonable candidate for holding places for all > these information needed in the internal implementation. > > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> > --- > fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------ > include/linux/netfs.h | 7 ++++++ > include/trace/events/cachefiles.h | 4 +-- > 3 files changed, 43 insertions(+), 10 deletions(-) > > diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c > index 000a28f46e59..6427259fcba9 100644 > --- a/fs/cachefiles/io.c > +++ b/fs/cachefiles/io.c > @@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres, > term_func, term_func_priv); > } > > -/* > - * Prepare a read operation, shortening it to a cached/uncached > - * boundary as appropriate. > - */ > -static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, > - loff_t i_size) > +static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq, > + struct netfs_cache_resources *cres, > + loff_t i_size) > { > enum cachefiles_prepare_read_trace why; > - struct netfs_io_request *rreq = subreq->rreq; > - struct netfs_cache_resources *cres = &rreq->cache_resources; > struct cachefiles_object *object; > struct cachefiles_cache *cache; > struct fscache_cookie *cookie = fscache_cres_cookie(cres); > @@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest * > return ret; > } > > +/* > + * Prepare a read operation, shortening it to a cached/uncached > + * boundary as appropriate. > + */ > +static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, > + loff_t i_size) > +{ > + return cachefiles_do_prepare_read(subreq, > + &subreq->rreq->cache_resources, i_size); > +} > + > +/* > + * Prepare an on-demand read operation, shortening it to a cached/uncached > + * boundary as appropriate. > + */ > +static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres, > + loff_t start, size_t *_len, loff_t i_size) > +{ > + enum netfs_io_source source; > + struct netfs_io_subrequest subreq = { > + .start = start, > + .len = *_len, > + .flags = 1 << NETFS_SREQ_ONDEMAND, > + }; > + Faking up a struct like this is sort of fragile. What if we change cachefiles_do_prepare_read to consult other fields in this structure later? It might be best to have cachefiles_do_prepare_read take individual start, len, and flags values instead of doing this. > + source = cachefiles_do_prepare_read(&subreq, cres, i_size); > + *_len = subreq.len; > + return source; > +} > + > /* > * Prepare for a write to occur. > */ > @@ -621,6 +646,7 @@ static const struct netfs_cache_ops cachefiles_netfs_cache_ops = { > .write = cachefiles_write, > .prepare_read = cachefiles_prepare_read, > .prepare_write = cachefiles_prepare_write, > + .prepare_ondemand_read = cachefiles_prepare_ondemand_read, > .query_occupancy = cachefiles_query_occupancy, > }; > > diff --git a/include/linux/netfs.h b/include/linux/netfs.h > index f2402ddeafbf..d82071c37133 100644 > --- a/include/linux/netfs.h > +++ b/include/linux/netfs.h > @@ -267,6 +267,13 @@ struct netfs_cache_ops { > loff_t *_start, size_t *_len, loff_t i_size, > bool no_space_allocated_yet); > > + /* Prepare an on-demand read operation, shortening it to a cached/uncached > + * boundary as appropriate. > + */ > + enum netfs_io_source (*prepare_ondemand_read)(struct netfs_cache_resources *cres, > + loff_t start, size_t *_len, > + loff_t i_size); > + > /* Query the occupancy of the cache in a region, returning where the > * next chunk of data starts and how long it is. > */ > diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h > index d8d4d73fe7b6..655d5900b8ef 100644 > --- a/include/trace/events/cachefiles.h > +++ b/include/trace/events/cachefiles.h > @@ -448,14 +448,14 @@ TRACE_EVENT(cachefiles_prep_read, > ), > > TP_fast_assign( > - __entry->rreq = sreq->rreq->debug_id; > + __entry->rreq = sreq->rreq ? sreq->rreq->debug_id : 0; > __entry->index = sreq->debug_index; > __entry->flags = sreq->flags; > __entry->source = source; > __entry->why = why; > __entry->len = sreq->len; > __entry->start = sreq->start; > - __entry->netfs_inode = sreq->rreq->inode->i_ino; > + __entry->netfs_inode = sreq->rreq ? sreq->rreq->inode->i_ino : 0; > __entry->cache_inode = cache_inode; > ), >
On 11/4/22 7:18 PM, Jeff Layton wrote: > On Fri, 2022-11-04 at 15:26 +0800, Jingbo Xu wrote: >> Add prepare_ondemand_read() callback dedicated for the on-demand read >> scenario, so that callers from this scenario can be decoupled from >> netfs_io_subrequest. >> >> To reuse the hole detecting logic as mush as possible, both the >> implementation of prepare_read() and prepare_ondemand_read() inside >> Cachefiles call a common routine. >> >> In the near future, prepare_read() will get enhanced and more >> information will be needed and then returned to callers. Thus >> netfs_io_subrequest is a reasonable candidate for holding places for all >> these information needed in the internal implementation. >> >> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> >> --- >> fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------ >> include/linux/netfs.h | 7 ++++++ >> include/trace/events/cachefiles.h | 4 +-- >> 3 files changed, 43 insertions(+), 10 deletions(-) >> >> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c >> index 000a28f46e59..6427259fcba9 100644 >> --- a/fs/cachefiles/io.c >> +++ b/fs/cachefiles/io.c >> @@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres, >> term_func, term_func_priv); >> } >> >> -/* >> - * Prepare a read operation, shortening it to a cached/uncached >> - * boundary as appropriate. >> - */ >> -static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, >> - loff_t i_size) >> +static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq, >> + struct netfs_cache_resources *cres, >> + loff_t i_size) >> { >> enum cachefiles_prepare_read_trace why; >> - struct netfs_io_request *rreq = subreq->rreq; >> - struct netfs_cache_resources *cres = &rreq->cache_resources; >> struct cachefiles_object *object; >> struct cachefiles_cache *cache; >> struct fscache_cookie *cookie = fscache_cres_cookie(cres); >> @@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest * >> return ret; >> } >> >> +/* >> + * Prepare a read operation, shortening it to a cached/uncached >> + * boundary as appropriate. >> + */ >> +static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, >> + loff_t i_size) >> +{ >> + return cachefiles_do_prepare_read(subreq, >> + &subreq->rreq->cache_resources, i_size); >> +} >> + >> +/* >> + * Prepare an on-demand read operation, shortening it to a cached/uncached >> + * boundary as appropriate. >> + */ >> +static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres, >> + loff_t start, size_t *_len, loff_t i_size) >> +{ >> + enum netfs_io_source source; >> + struct netfs_io_subrequest subreq = { >> + .start = start, >> + .len = *_len, >> + .flags = 1 << NETFS_SREQ_ONDEMAND, >> + }; >> + > > Faking up a struct like this is sort of fragile. What if we change > cachefiles_do_prepare_read to consult other fields in this structure > later? Indeed it's not robust somehow. > > It might be best to have cachefiles_do_prepare_read take individual > start, len, and flags values instead of doing this. > I will give it a try if nobody objects this. Thank you for your suggestions :)
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c index 000a28f46e59..6427259fcba9 100644 --- a/fs/cachefiles/io.c +++ b/fs/cachefiles/io.c @@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres, term_func, term_func_priv); } -/* - * Prepare a read operation, shortening it to a cached/uncached - * boundary as appropriate. - */ -static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, - loff_t i_size) +static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq, + struct netfs_cache_resources *cres, + loff_t i_size) { enum cachefiles_prepare_read_trace why; - struct netfs_io_request *rreq = subreq->rreq; - struct netfs_cache_resources *cres = &rreq->cache_resources; struct cachefiles_object *object; struct cachefiles_cache *cache; struct fscache_cookie *cookie = fscache_cres_cookie(cres); @@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest * return ret; } +/* + * Prepare a read operation, shortening it to a cached/uncached + * boundary as appropriate. + */ +static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq, + loff_t i_size) +{ + return cachefiles_do_prepare_read(subreq, + &subreq->rreq->cache_resources, i_size); +} + +/* + * Prepare an on-demand read operation, shortening it to a cached/uncached + * boundary as appropriate. + */ +static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres, + loff_t start, size_t *_len, loff_t i_size) +{ + enum netfs_io_source source; + struct netfs_io_subrequest subreq = { + .start = start, + .len = *_len, + .flags = 1 << NETFS_SREQ_ONDEMAND, + }; + + source = cachefiles_do_prepare_read(&subreq, cres, i_size); + *_len = subreq.len; + return source; +} + /* * Prepare for a write to occur. */ @@ -621,6 +646,7 @@ static const struct netfs_cache_ops cachefiles_netfs_cache_ops = { .write = cachefiles_write, .prepare_read = cachefiles_prepare_read, .prepare_write = cachefiles_prepare_write, + .prepare_ondemand_read = cachefiles_prepare_ondemand_read, .query_occupancy = cachefiles_query_occupancy, }; diff --git a/include/linux/netfs.h b/include/linux/netfs.h index f2402ddeafbf..d82071c37133 100644 --- a/include/linux/netfs.h +++ b/include/linux/netfs.h @@ -267,6 +267,13 @@ struct netfs_cache_ops { loff_t *_start, size_t *_len, loff_t i_size, bool no_space_allocated_yet); + /* Prepare an on-demand read operation, shortening it to a cached/uncached + * boundary as appropriate. + */ + enum netfs_io_source (*prepare_ondemand_read)(struct netfs_cache_resources *cres, + loff_t start, size_t *_len, + loff_t i_size); + /* Query the occupancy of the cache in a region, returning where the * next chunk of data starts and how long it is. */ diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h index d8d4d73fe7b6..655d5900b8ef 100644 --- a/include/trace/events/cachefiles.h +++ b/include/trace/events/cachefiles.h @@ -448,14 +448,14 @@ TRACE_EVENT(cachefiles_prep_read, ), TP_fast_assign( - __entry->rreq = sreq->rreq->debug_id; + __entry->rreq = sreq->rreq ? sreq->rreq->debug_id : 0; __entry->index = sreq->debug_index; __entry->flags = sreq->flags; __entry->source = source; __entry->why = why; __entry->len = sreq->len; __entry->start = sreq->start; - __entry->netfs_inode = sreq->rreq->inode->i_ino; + __entry->netfs_inode = sreq->rreq ? sreq->rreq->inode->i_ino : 0; __entry->cache_inode = cache_inode; ),
Add prepare_ondemand_read() callback dedicated for the on-demand read scenario, so that callers from this scenario can be decoupled from netfs_io_subrequest. To reuse the hole detecting logic as mush as possible, both the implementation of prepare_read() and prepare_ondemand_read() inside Cachefiles call a common routine. In the near future, prepare_read() will get enhanced and more information will be needed and then returned to callers. Thus netfs_io_subrequest is a reasonable candidate for holding places for all these information needed in the internal implementation. Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com> --- fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------ include/linux/netfs.h | 7 ++++++ include/trace/events/cachefiles.h | 4 +-- 3 files changed, 43 insertions(+), 10 deletions(-)