Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: unlock client_lock when copying data and do more check for the client_lock #36730

Merged
merged 4 commits into from Sep 28, 2020

Conversation

lxbsz
Copy link
Member

@lxbsz lxbsz commented Aug 20, 2020

Unlock client_lock when copying data to bufferlist And avoid using the client_lock.unlock/.lock directly if possible, and add more check for the client_lock.

Fixes: https://tracker.ceph.com/issues/47039
Fixes: https://tracker.ceph.com/issues/47047
Signed-off-by: Xiubo Li xiubli@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

src/client/Client.cc Outdated Show resolved Hide resolved
src/client/Client.cc Show resolved Hide resolved
src/client/Client.h Outdated Show resolved Hide resolved
Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

src/client/Client.cc Show resolved Hide resolved
src/client/Client.cc Show resolved Hide resolved
src/client/Client.cc Show resolved Hide resolved
src/client/Client.h Show resolved Hide resolved
src/client/Client.cc Outdated Show resolved Hide resolved
@lxbsz lxbsz force-pushed the nolockcpy branch 2 times, most recently from bca39e0 to be91dc1 Compare September 23, 2020 01:52
@lxbsz
Copy link
Member Author

lxbsz commented Sep 23, 2020

@batrick
Fixed them all, thanks.

src/client/Client.cc Outdated Show resolved Hide resolved
@batrick
Copy link
Member

batrick commented Sep 24, 2020

/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:9756:9: error: no declaration matches 'int64_t Client::_preadv_pwritev_locked(Fh*, const iovec*, unsigned int, int64_t, bool, bool, std::unique_lock<ceph::mutex_debug_detail::mutex_debug_impl<false> >&)'
 9756 | int64_t Client::_preadv_pwritev_locked(Fh *fh, const struct iovec *iov,
      |         ^~~~~~
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:89:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note: candidate is: 'int64_t Client::_preadv_pwritev_locked(Fh*, const iovec*, unsigned int, int64_t, bool, bool)'
 1237 |   int64_t _preadv_pwritev_locked(Fh *f, const struct iovec *iov,
      |           ^~~~~~~~~~~~~~~~~~~~~~
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:89:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:230:7: note: 'class Client' defined here
  230 | class Client : public Dispatcher, public md_config_obs_t {
      |       ^~~~~~
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc: In member function 'int Client::_preadv_pwritev(int, const iovec*, unsigned int, int64_t, bool)':
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:9819:75: error: no matching function for call to 'Client::_preadv_pwritev_locked(Fh*&, const iovec*&, unsigned int&, int64_t&, bool&, bool, std::unique_lock<ceph::mutex_debug_detail::mutex_debug_impl<false> >&)'
 9819 |     return _preadv_pwritev_locked(fh, iov, iovcnt, offset, write, true, cl);
      |                                                                           ^
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:89:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note: candidate: 'int64_t Client::_preadv_pwritev_locked(Fh*, const iovec*, unsigned int, int64_t, bool, bool)'
 1237 |   int64_t _preadv_pwritev_locked(Fh *f, const struct iovec *iov,
      |           ^~~~~~~~~~~~~~~~~~~~~~
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note:   candidate expects 6 arguments, 7 provided
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc: In member function 'int64_t Client::ll_writev(Fh*, const iovec*, int, int64_t)':
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:13851:70: error: no matching function for call to 'Client::_preadv_pwritev_locked(Fh*&, const iovec*&, int&, int64_t&, bool, bool, std::unique_lock<ceph::mutex_debug_detail::mutex_debug_impl<false> >&)'
13851 |   return _preadv_pwritev_locked(fh, iov, iovcnt, off, true, false, cl);
      |                                                                      ^
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:89:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note: candidate: 'int64_t Client::_preadv_pwritev_locked(Fh*, const iovec*, unsigned int, int64_t, bool, bool)'
 1237 |   int64_t _preadv_pwritev_locked(Fh *f, const struct iovec *iov,
      |           ^~~~~~~~~~~~~~~~~~~~~~
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note:   candidate expects 6 arguments, 7 provided
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc: In member function 'int64_t Client::ll_readv(Fh*, const iovec*, int, int64_t)':
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:13861:71: error: no matching function for call to 'Client::_preadv_pwritev_locked(Fh*&, const iovec*&, int&, int64_t&, bool, bool, std::unique_lock<ceph::mutex_debug_detail::mutex_debug_impl<false> >&)'
13861 |   return _preadv_pwritev_locked(fh, iov, iovcnt, off, false, false, cl);
      |                                                                       ^
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.cc:89:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note: candidate: 'int64_t Client::_preadv_pwritev_locked(Fh*, const iovec*, unsigned int, int64_t, bool, bool)'
 1237 |   int64_t _preadv_pwritev_locked(Fh *f, const struct iovec *iov,
      |           ^~~~~~~~~~~~~~~~~~~~~~
/home/jenkins-build/build/workspace/ceph-pull-requests/src/client/Client.h:1237:11: note:   candidate expects 6 arguments, 7 provided
src/client/CMakeFiles/client.dir/build.make:62: recipe for target 'src/client/CMakeFiles/client.dir/Client.cc.o' failed
make[3]: *** [src/client/CMakeFiles/client.dir/Client.cc.o] Error 1
CMakeFiles/Makefile2:30648: recipe for target 'src/client/CMakeFiles/client.dir/all' failed

Fixes: https://tracker.ceph.com/issues/47039
Signed-off-by: Xiubo Li <xiubli@redhat.com>
It's no need to hold the lock when copying the data, which may
take a long time.

Fixes: https://tracker.ceph.com/issues/47047
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
Copy link
Member Author

lxbsz commented Sep 25, 2020

Sorry, the client.h changes still in the git stash, will update it soon.

@batrick
Copy link
Member

batrick commented Sep 28, 2020

@batrick batrick merged commit c7b6d8a into ceph:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants