tools/cephfs-mirror: eliminate redundant ceph_close() call#60667
tools/cephfs-mirror: eliminate redundant ceph_close() call#60667
Conversation
|
jenkins test api |
|
@joscollin PTAL for review and approve if the change looks good. |
|
@vshankar Will check soon. |
|
jenkins test api |
Anything on this @joscollin ? |
Currently reviewing it. The new changes looks good at the first look. Checking in detail. |
5dc24fa to
b9d40b1
Compare
|
|
joscollin
left a comment
There was a problem hiding this comment.
Otherwise, looks good to me.
b9d40b1 to
8ef5012
Compare
joscollin
left a comment
There was a problem hiding this comment.
Now, dout is only for success case. But not a big deal. LGTM.
8ef5012 to
4234eb7
Compare
Likely ceph#55619 missed that. Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
ceph#55619 eliminates the need for ceph_close() call after successful ceph_fdopendir() one. And introduces automatic file descriptor's close when corresponding ceph_closedir() is called. That hasty ceph_close() call makes file descriptor available for a new allocation which might conflict with the automatic fd close in the above ceph_closedir(). Full PeerReplayer::do_synchronize() has been reworked to close fds properly, depending on whether ceph_fdopendir() has been already applied to them. Additionally for the sake of uniformity this reworks incremental do_synchronize() in a way to do final fd closings similar to full implementation. Plus this effectively reverts ceph@bd78bdc as it looks like a wrong approach to fight broken file descriptor references. No much sense in reopening of the current snapshot's root folder on each new entry processing. Instead this patch just doesn't close it from the beginning. Fixes: https://tracker.ceph.com/issues/68853 Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
4234eb7 to
8c5999f
Compare
@joscollin - yeah, squashed commits around ceph_close() into a single one. |
Thanks.
We won't catch that in the fs:mirror tests, but it certainly needs QA. So could you please create a new PR for that fix? |
8c5999f to
7a747bc
Compare
Done |
joscollin
left a comment
There was a problem hiding this comment.
LGTM.
@rishabh-d-dave Could you please add this PR to your next QA batch run? Thx.
|
jenkins test api |
|
@rishabh-d-dave This is a known issue [1] https://tracker.ceph.com/issues/68567. |
|
@rishabh-d-dave This is a known issue, which is tracked here: https://tracker.ceph.com/issues/68567. Could you please drop that failed test and QA again, so that we could get this PR in? |
Drop that test how?
QA again why? We already did a full QA run, there are no other mirror related failures there. |
* refs/pull/60667/head: cephfs-mirror: remove redundant ceph_close() calls. cephfs/client: dir_reset_t::reset() - add missing fd reset. Reviewed-by: Jos Collin <jcollin@redhat.com>
#61068 |
|
@vshankar Please merge this PR. |
Fixes: https://tracker.ceph.com/issues/68853
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windowsjenkins test rook e2e