| Message ID | 20211227125444.21187-20-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fscache,erofs: fscache-based demand-read semantics | expand |
Hi Jeffle,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on dhowells-fs/fscache-next]
[cannot apply to xiang-erofs/dev-test ceph-client/for-linus linus/master v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jeffle-Xu/fscache-erofs-fscache-based-demand-read-semantics/20211227-205742
base: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git fscache-next
config: csky-defconfig (https://download.01.org/0day-ci/archive/20211227/202112272340.1ho7sEjG-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/54c300b4598e3f2836d8233681da387fe388cfda
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jeffle-Xu/fscache-erofs-fscache-based-demand-read-semantics/20211227-205742
git checkout 54c300b4598e3f2836d8233681da387fe388cfda
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=csky SHELL=/bin/bash fs/cachefiles/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/cachefiles/io.c:564:5: warning: no previous prototype for 'cachefiles_demand_read' [-Wmissing-prototypes]
564 | int cachefiles_demand_read(struct netfs_cache_resources *cres,
| ^~~~~~~~~~~~~~~~~~~~~~
In function 'cachefiles_alloc_req',
inlined from 'cachefiles_demand_read' at fs/cachefiles/io.c:575:8:
>> fs/cachefiles/io.c:557:9: warning: 'strncpy' specified bound 255 equals destination size [-Wstringop-truncation]
557 | strncpy(req_in->path, object->d_name, sizeof(req_in->path));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/strncpy +557 fs/cachefiles/io.c
541
542 static struct cachefiles_req *cachefiles_alloc_req(struct cachefiles_object *object,
543 loff_t start_pos,
544 size_t len)
545 {
546 struct cachefiles_req *req;
547 struct cachefiles_req_in *req_in;
548
549 req = kzalloc(sizeof(*req), GFP_KERNEL);
550 if (!req)
551 return NULL;
552
553 req_in = &req->req_in;
554
555 req_in->off = start_pos;
556 req_in->len = len;
> 557 strncpy(req_in->path, object->d_name, sizeof(req_in->path));
558
559 init_completion(&req->done);
560
561 return req;
562 }
563
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Dec 27, 2021 at 08:54:40PM +0800, Jeffle Xu wrote: > + spin_lock(&cache->reqs_lock); > + ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL); GFP_KERNEL while holding a spinlock? You should be using an XArray instead of an IDR in new code anyway.
Hi Jeffle,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on dhowells-fs/fscache-next]
[cannot apply to xiang-erofs/dev-test ceph-client/for-linus linus/master v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jeffle-Xu/fscache-erofs-fscache-based-demand-read-semantics/20211227-205742
base: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git fscache-next
config: i386-randconfig-r005-20211227 (https://download.01.org/0day-ci/archive/20211227/202112272353.ekKESQX6-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 511726c64d3b6cca66f7c54d457d586aa3129f67)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/54c300b4598e3f2836d8233681da387fe388cfda
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jeffle-Xu/fscache-erofs-fscache-based-demand-read-semantics/20211227-205742
git checkout 54c300b4598e3f2836d8233681da387fe388cfda
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/cachefiles/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/cachefiles/io.c:564:5: warning: no previous prototype for function 'cachefiles_demand_read' [-Wmissing-prototypes]
int cachefiles_demand_read(struct netfs_cache_resources *cres,
^
fs/cachefiles/io.c:564:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int cachefiles_demand_read(struct netfs_cache_resources *cres,
^
static
1 warning generated.
vim +/cachefiles_demand_read +564 fs/cachefiles/io.c
563
> 564 int cachefiles_demand_read(struct netfs_cache_resources *cres,
565 loff_t start_pos, size_t len)
566 {
567 struct cachefiles_object *object;
568 struct cachefiles_cache *cache;
569 struct cachefiles_req *req;
570 int ret;
571
572 object = cachefiles_cres_object(cres);
573 cache = object->volume->cache;
574
575 req = cachefiles_alloc_req(object, start_pos, len);
576 if (!req)
577 return -ENOMEM;
578
579 spin_lock(&cache->reqs_lock);
580 ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL);
581 if (ret >= 0)
582 req->req_in.id = ret;
583 spin_unlock(&cache->reqs_lock);
584 if (ret < 0) {
585 kfree(req);
586 return -ENOMEM;
587 }
588
589 wake_up_all(&cache->daemon_pollwq);
590
591 wait_for_completion(&req->done);
592 kfree(req);
593
594 return 0;
595 }
596
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 12/27/21 11:36 PM, Matthew Wilcox wrote: > On Mon, Dec 27, 2021 at 08:54:40PM +0800, Jeffle Xu wrote: >> + spin_lock(&cache->reqs_lock); >> + ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL); > > GFP_KERNEL while holding a spinlock? Right. Thanks for pointing it out. > > You should be using an XArray instead of an IDR in new code anyway. > Regards.
On 12/28/21 8:33 PM, JeffleXu wrote: > > > On 12/27/21 11:36 PM, Matthew Wilcox wrote: >> On Mon, Dec 27, 2021 at 08:54:40PM +0800, Jeffle Xu wrote: >>> + spin_lock(&cache->reqs_lock); >>> + ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL); >> >> GFP_KERNEL while holding a spinlock? > > Right. Thanks for pointing it out. > >> >> You should be using an XArray instead of an IDR in new code anyway. >> > > Regards. > Hi Matthew, I'm afraid IDR can't be replaced by xarray here. Because we need an 'ID' for each pending read request, so that after fetching data from remote, user daemon could notify kernel which read request has finished by this 'ID'. Currently this 'ID' is get from idr_alloc(), and actually identifies the position of corresponding read request inside the IDR tree. I can't find similar API of xarray implementing similar function, i.e., returning an 'ID'. As for the issue of GFP_KERNEL while holding a spinlock, I'm going to fix this with idr_preload(), somehing like + idr_preload(GFP_KERNEL); + idr_lock(&cache->reqs); + + ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_ATOMIC); + if (ret >= 0) + req->req_in.id = ret; + + idr_unlock(&cache->reqs); + idr_preload_end(); Please correct me if I'm wrong.
On Wed, Jan 12, 2022 at 05:02:13PM +0800, JeffleXu wrote: > I'm afraid IDR can't be replaced by xarray here. Because we need an 'ID' > for each pending read request, so that after fetching data from remote, > user daemon could notify kernel which read request has finished by this > 'ID'. > > Currently this 'ID' is get from idr_alloc(), and actually identifies the > position of corresponding read request inside the IDR tree. I can't find > similar API of xarray implementing similar function, i.e., returning an > 'ID'. xa_alloc().
On 1/19/22 9:20 PM, Matthew Wilcox wrote: > On Wed, Jan 12, 2022 at 05:02:13PM +0800, JeffleXu wrote: >> I'm afraid IDR can't be replaced by xarray here. Because we need an 'ID' >> for each pending read request, so that after fetching data from remote, >> user daemon could notify kernel which read request has finished by this >> 'ID'. >> >> Currently this 'ID' is get from idr_alloc(), and actually identifies the >> position of corresponding read request inside the IDR tree. I can't find >> similar API of xarray implementing similar function, i.e., returning an >> 'ID'. > > xa_alloc(). > Oh yes. Thanks. I will try to convert to xarray API...
diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h index af8ac8107f77..eeb6ad7dcd49 100644 --- a/fs/cachefiles/internal.h +++ b/fs/cachefiles/internal.h @@ -61,6 +61,18 @@ struct cachefiles_object { #define CACHEFILES_OBJECT_USING_TMPFILE 0 /* Have an unlinked tmpfile */ }; +struct cachefiles_req_in { + uint64_t id; + uint64_t off; + uint64_t len; + char path[NAME_MAX]; +}; + +struct cachefiles_req { + struct completion done; + struct cachefiles_req_in req_in; +}; + /* * Cache files cache definition */ diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c index 60b1eac2ce78..376603e5ed99 100644 --- a/fs/cachefiles/io.c +++ b/fs/cachefiles/io.c @@ -539,12 +539,68 @@ static void cachefiles_end_operation(struct netfs_cache_resources *cres) fscache_end_cookie_access(fscache_cres_cookie(cres), fscache_access_io_end); } +static struct cachefiles_req *cachefiles_alloc_req(struct cachefiles_object *object, + loff_t start_pos, + size_t len) +{ + struct cachefiles_req *req; + struct cachefiles_req_in *req_in; + + req = kzalloc(sizeof(*req), GFP_KERNEL); + if (!req) + return NULL; + + req_in = &req->req_in; + + req_in->off = start_pos; + req_in->len = len; + strncpy(req_in->path, object->d_name, sizeof(req_in->path)); + + init_completion(&req->done); + + return req; +} + +int cachefiles_demand_read(struct netfs_cache_resources *cres, + loff_t start_pos, size_t len) +{ + struct cachefiles_object *object; + struct cachefiles_cache *cache; + struct cachefiles_req *req; + int ret; + + object = cachefiles_cres_object(cres); + cache = object->volume->cache; + + req = cachefiles_alloc_req(object, start_pos, len); + if (!req) + return -ENOMEM; + + spin_lock(&cache->reqs_lock); + ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL); + if (ret >= 0) + req->req_in.id = ret; + spin_unlock(&cache->reqs_lock); + if (ret < 0) { + kfree(req); + return -ENOMEM; + } + + wake_up_all(&cache->daemon_pollwq); + + wait_for_completion(&req->done); + kfree(req); + + return 0; +} + static const struct netfs_cache_ops cachefiles_netfs_cache_ops = { .end_operation = cachefiles_end_operation, .read = cachefiles_read, .write = cachefiles_write, .prepare_read = cachefiles_prepare_read, .prepare_write = cachefiles_prepare_write, + .demand_read = cachefiles_demand_read, }; /*
Implement .demand_read() callback for cachefiels backend. .demand_read() callback is responsible for notifying user daemon the pending request to process, and will get blocked until user daemon has prepared data and filled the hole. Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com> --- fs/cachefiles/internal.h | 12 +++++++++ fs/cachefiles/io.c | 56 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)