kraken: rgw: rgw_file: FHCache residence check should be exhaustive #13841

smithfarm · 2017-03-07T21:11:07Z

http://tracker.ceph.com/issues/19144
http://tracker.ceph.com/issues/19723
http://tracker.ceph.com/issues/20195

smithfarm · 2017-04-07T12:59:50Z

Jenkins retest this please (logs gone)

smithfarm · 2017-04-07T14:12:27Z

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_os_lib.cc:8:0:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_file.h: In member function ‘rgw::LookupFHResult rgw::RGWLibFS::lookup_fh(const rgw::fh_key&, uint32_t)’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_file.h:897:20: error: cannot bind ‘std::basic_ostream<char>’ lvalue to ‘std::basic_ostream<char>&&’
        << __func__ << " 1 " << *fh
                    ^
In file included from /usr/include/c++/4.8/iterator:64:0,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/include/boost/utility/string_ref.hpp:26,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_rest.h:9,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_os_lib.cc:4:
/usr/include/c++/4.8/ostream:602:5: error:   initializing argument 1 of ‘std::basic_ostream<_CharT, _Traits>& std::operator<<(std::basic_ostream<_CharT, _Traits>&&, const _Tp&) [with _CharT = char; _Traits = std::char_traits<char>; _Tp = rgw::RGWFileHandle]’
     operator<<(basic_ostream<_CharT, _Traits>&& __os, const _Tp& __x)
     ^
In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_os_lib.cc:8:0:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_file.h: In member function ‘rgw::LookupFHResult rgw::RGWLibFS::lookup_fh(rgw::RGWFileHandle*, const char*, uint32_t)’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_file.h:980:20: error: cannot bind ‘std::basic_ostream<char>’ lvalue to ‘std::basic_ostream<char>&&’
        << __func__ << " 2 " << *fh
                    ^
In file included from /usr/include/c++/4.8/iterator:64:0,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/build/boost/include/boost/utility/string_ref.hpp:26,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_rest.h:9,
                 from /home/jenkins-build/build/workspace/ceph-pull-requests/src/rgw/rgw_os_lib.cc:4:
/usr/include/c++/4.8/ostream:602:5: error:   initializing argument 1 of ‘std::basic_ostream<_CharT, _Traits>& std::operator<<(std::basic_ostream<_CharT, _Traits>&&, const _Tp&) [with _CharT = char; _Traits = std::char_traits<char>; _Tp = rgw::RGWFileHandle]’
     operator<<(basic_ostream<_CharT, _Traits>&& __os, const _Tp& __x)
     ^
[ 83%] Building CXX object src/rgw/CMakeFiles/rgw_a.dir/rgw_process.cc.o
make[3]: *** [src/rgw/CMakeFiles/rgw_a.dir/rgw_os_lib.cc.o] Error 1

@mattbenjamin: Whelp!

mattbenjamin · 2017-06-06T05:40:51Z

@will test build...

mattbenjamin · 2017-06-06T07:41:43Z

@smithfarm the handle print requires having merged 1d6c72f

mattbenjamin

approve modulo it builds :)

smithfarm · 2017-06-06T07:46:51Z

@mattbenjamin Thanks!

mattbenjamin · 2017-06-06T08:18:34Z

@smithfarm sorry, we're not missing 1d6c72f, but need to understand the conflict (want to ensure that correct handle lifecycle commits all land); looks like empty dtor ~RGWFileHandle is correct

smithfarm · 2017-06-06T08:21:14Z

@mattbenjamin No, not 1d6c72f (which is a jewel backport), but I did cherry-pick ef330f3 into this PR which will hopefully resolve the build issue.

smithfarm · 2017-06-06T08:22:27Z

oh, now I see the conflict. I'll re-do that commit as well.

mattbenjamin · 2017-06-06T08:25:12Z

@smithfarm basically kraken should I think get backports of all commits that reached jewel after kraken branched; kraken seems to be missing at least the following:
70ef7d4
2e66c7a

smithfarm · 2017-06-06T08:27:03Z

@mattbenjamin First, I redid the cherry-picks, so the conflict is gone. Second, I tried cherry-picking 70ef7d4 and 2e66c7a on top of this PR and neither cherry-picks cleanly.

mattbenjamin · 2017-06-06T08:28:48Z

@smithfarm I did search those on a jewel branch, I'll re-run on master; however, I do know that LIBRGW_FILE_VER_EXTRA (src/include/rados/rgw_file.h) is incorrect on Kraken

mattbenjamin · 2017-06-06T08:58:07Z

@smithfarm proposed conflict resolutions on:
git@github.com:linuxbox2/ceph.git smithfarm-wip-19144-kraken
(ignore top commit)

smithfarm · 2017-06-06T10:02:46Z

@mattbenjamin OK, integrated those fixes, updated trackers etc.

mattbenjamin

lgtm

smithfarm · 2017-06-06T10:38:02Z

Jenkins re-test this please

RGW NFS fhcache/RGWFileHandle operators assume existence of the full chain of parents from any object to the its fs_root--this is a consequence of the weakly-connected namespace design goal, and not a defect. This change ensures the invariant by taking a parent ref when objects are interned (when a parent ref is guaranteed). Parent refs are returned when objects are destroyed--essentially by the invariant, such a ref must exist. The extra ref is omitted when parent->is_root(), as that node is not in the LRU cache. Fixes: http://tracker.ceph.com/issues/18650 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit 0e5299f)

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit ef330f3)

These are helpful for checking RGWFileHandle refcnt invariants. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit 462034e)

This change includes 3 related changes: 1. add required lock flags for FHCache updates--this is a crash bug under concurrent update/lookup 2. omit to inc/dec refcnt on root filehandles in 2 places--the root handle current is not on the lru list, so it's not valid to do so 3. based on observation of LRU behavior during creates/deletes, update (cohort) LRU unref to move objects to LRU when their refcount falls to SENTINEL_REFCNT--this cheaply primes the current reclaim() mechanism, so very significanty improves space use (e.g., after deletes) in the absence of scans (which is common due to nfs-ganesha caching) Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit beaeff0)

Previously we assumed that !deleted handles were resident--there is an observed case where a !deleted handle is !linked. Since we currently use safe_link mode, an is_linked() check is available, and exhaustive. Fixes: http://tracker.ceph.com/issues/19111 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit c0aa515)

Allow callers of rgw_lookup() on objects attested in an rgw_readdir() callback the ability to bypass exact match in RGWLibFS::stat_leaf() case 2, but restore exact match enforcement for general lookups. This preserves required common_prefix namespace behavior, but prevents clients from eerily permitting things like "cd sara0" via partial name match on "sara01." Fixes: http://tracker.ceph.com/issues/19059 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit 70ef7d4) Conflicts: src/include/rados/rgw_file.h: trivial resolution src/rgw/rgw_file.cc: trivial resolution

The new type hints optimize object type deduction, when the rgw_lookup is called from an rgw_readdir callback. Fixes: http://tracker.ceph.com/issues/19623 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit 2e66c7a) Conflicts: src/rgw/rgw_file.h: trivial resolution

smithfarm · 2017-07-04T07:20:06Z

@mattbenjamin In the end I merged this PR into #13871. The two commits that had conflicts (which you resolved) no longer conflicted when I cherry-picked them on top of the commits already in that PR.

mattbenjamin · 2017-07-05T16:38:13Z

ok, cool

…

On Tue, Jul 4, 2017 at 3:20 AM, Nathan Cutler ***@***.***> wrote: @mattbenjamin <https://github.com/mattbenjamin> In the end I merged this PR into #13871 <#13871>. The two commits that had conflicts (which you resolved) no longer conflicted when I cherry-picked them on top of the commits already in that PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13841 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASY58wgS5E_-3U23JtTbIbN_ZK21DRRks5sKeengaJpZM4MWB6D> .

smithfarm self-assigned this Mar 7, 2017

smithfarm added this to the kraken milestone Mar 7, 2017

smithfarm added bug-fix core labels Mar 7, 2017

smithfarm requested a review from mattbenjamin March 7, 2017 21:11

smithfarm added rgw and removed core labels Mar 7, 2017

smithfarm changed the title ~~kraken: rgw_file: FHCache residence check should be exhaustive~~ [DNM] kraken: rgw_file: FHCache residence check should be exhaustive Apr 7, 2017

mattbenjamin reviewed Jun 6, 2017

View reviewed changes

smithfarm force-pushed the wip-19144-kraken branch from 7a224a9 to 97ebfde Compare June 6, 2017 07:46

smithfarm changed the title ~~[DNM] kraken: rgw_file: FHCache residence check should be exhaustive~~ kraken: rgw: rgw_file: FHCache residence check should be exhaustive Jun 6, 2017

smithfarm force-pushed the wip-19144-kraken branch from 97ebfde to 3ee8776 Compare June 6, 2017 08:25

smithfarm force-pushed the wip-19144-kraken branch from bca21e8 to e349a39 Compare June 6, 2017 09:56

mattbenjamin approved these changes Jun 6, 2017

View reviewed changes

mattbenjamin added 2 commits July 3, 2017 16:35

rgw_file: add pretty-print for RGWFileHandle

47f5332

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit ef330f3)

mattbenjamin added 5 commits July 3, 2017 16:35

rgw_file: add refcount dout traces at debuglevel 17

490b495

These are helpful for checking RGWFileHandle refcnt invariants. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com> (cherry picked from commit 462034e)

smithfarm force-pushed the wip-19144-kraken branch from e349a39 to c46113e Compare July 3, 2017 14:35

smithfarm closed this Jul 4, 2017

smithfarm deleted the wip-19144-kraken branch July 4, 2017 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kraken: rgw: rgw_file: FHCache residence check should be exhaustive #13841

kraken: rgw: rgw_file: FHCache residence check should be exhaustive #13841

smithfarm commented Mar 7, 2017 •

edited

smithfarm commented Apr 7, 2017

smithfarm commented Apr 7, 2017

mattbenjamin commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017 •

edited

mattbenjamin left a comment

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017 •

edited

smithfarm commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin left a comment

smithfarm commented Jun 6, 2017

smithfarm commented Jul 4, 2017

mattbenjamin commented Jul 5, 2017 via email

kraken: rgw: rgw_file: FHCache residence check should be exhaustive #13841

kraken: rgw: rgw_file: FHCache residence check should be exhaustive #13841

Conversation

smithfarm commented Mar 7, 2017 • edited

smithfarm commented Apr 7, 2017

smithfarm commented Apr 7, 2017

mattbenjamin commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017 • edited

mattbenjamin left a comment

Choose a reason for hiding this comment

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017 • edited

smithfarm commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

mattbenjamin commented Jun 6, 2017

smithfarm commented Jun 6, 2017

mattbenjamin left a comment

Choose a reason for hiding this comment

smithfarm commented Jun 6, 2017

smithfarm commented Jul 4, 2017

mattbenjamin commented Jul 5, 2017 via email

smithfarm commented Mar 7, 2017 •

edited

mattbenjamin commented Jun 6, 2017 •

edited

mattbenjamin commented Jun 6, 2017 •

edited