client: do not proceed with I/O if filehandle is invalid by dparmar18 · Pull Request #55457 · ceph/ceph

dparmar18 · 2024-02-06T07:42:13Z

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 x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

src/client/Client.cc

src/test/client/nonblocking.cc

dparmar18 · 2024-02-09T13:27:41Z

last push made some minor naming corrections

mchangir · 2024-02-10T03:01:21Z

Why are low level interface methods being exercised rather than the usual high level methods ?

dparmar18 · 2024-02-12T07:31:59Z

Why are low level interface methods being exercised rather than the usual high level methods ?

Client::_preadv_pwritev_locked is called by I/O APIs, better to fix it here rather than doing the same everywhere

mchangir · 2024-02-12T08:23:43Z

Why are low level interface methods being exercised rather than the usual high level methods ?

Client::_preadv_pwritev_locked is called by I/O APIs, better to fix it here rather than doing the same everywhere

I meant to ask "Why are you explicitly invoking the low-level APIs directly in your test rather then invoking the high level entry points?"

dparmar18 · 2024-02-12T10:50:13Z

Why are low level interface methods being exercised rather than the usual high level methods ?

Client::_preadv_pwritev_locked is called by I/O APIs, better to fix it here rather than doing the same everywhere

I meant to ask "Why are you explicitly invoking the low-level APIs directly in your test rather then invoking the high level entry points?"

ah right, so it's the way nonblocking.cc is being used. it is intended to test ll_preadv_pwritev thoroughly. (atleast that is what im following looking at the existing test case)

vshankar · 2024-02-14T05:29:30Z

src/client/Client.cc

 {
    ceph_assert(ceph_mutex_is_locked_by_me(client_lock));

+    if (fh == NULL || !fh_exists(fh)) {


Have you checked other unchecked uses to the file handles?

... and this isn't really async specific, isn't it? (given that the test case is in nonblocking.cc).

... and this isn't really async specific, isn't it? (given that the test case is in nonblocking.cc).

right, not specific to async

Have you checked other unchecked uses to the file handles?

I found most cases handled in respective APIs, this seems to be the only condition(so far) that wasn't checked. If you can think of any/find any do let me know.

is it possible that a file handle has no inode associated with it?

... and this isn't really async specific, isn't it? (given that the test case is in nonblocking.cc).

right, not specific to async

Move the test to generic location then?

can goto src/test/libcephfs/test.cc

We have two kind of APIs:

that accept fd (an int) as an arg

that accept pointer to file handle as an arg. (sync + async)

Most APIs fall in 1st case and all of them have a check for file handle pointer's validness:

Fh *fh = get_filehandle(fd); if (!fh) return -CEPHFS_EBADF;

we already have tests for this in test.cc (test case named BadFileDesc). So the sync calls are sorted.

It is only the 2nd case where APIs directly make use of the file handle pointers and there are only three of them i.e. ll_preadv_pwritev, ll_writev and ll_readv.

My current implementation adds the check to helper func _preadv_pwritev_locked that all three of these use but the APIs that fall in 1st case make sure they check for it before passing in the fh to the helpers. So my question is what should be done, should I add these checks in all the three APIs (which i feel looks redundant) or keep it the way it is i.e. have the helper check for it? @mchangir @vshankar

My current implementation adds the check to helper func _preadv_pwritev_locked that all three of these use but the APIs that fall in 1st case make sure they check for it before passing in the fh to the helpers. So my question is what should be done, should I add these checks in all the three APIs (which i feel looks redundant) or keep it the way it is i.e. have the helper check for it? @mchangir @vshankar

Just have the fd checked in _preadv_pwritev_locked.

this breaks up higher level APIs, eventually every API calls Client::_preadv_pwritev_locked but the thing is high level APIs add fh to the fd_map while low level APIs make use of ll_unclosed_fh_set so if the file is opened using high level API like open or openat then those calls will fails since it wont be able to find the fh in ll_unclosed_fh_set as it was never added to it(the helper I added looks in ll_unclosed_fh_set)

github-actions · 2024-02-14T05:42:46Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2024-02-17T09:19:35Z

a fh pointer is stored in ll_unclosed_fh_set only if a file was opened using Client::ll_open or Client::ll_create. This is fine but let's think of a case where the client opens a file with the high level APIs like Client::open or Client::openat (which makes uses of Client::create_and_open which stores the fh pointer in fd_map); fetches the file handle and uses the retrieved pointer to feed into the low level API like Client::ll_writev and since ll_unclosed_fh_set has no fh in it(because file wasn't opened with low level APIs), we have a trouble here. Does this case(of using high level and low level APIs together) hold any ground @vshankar @mchangir ?

vshankar · 2024-02-20T04:37:25Z

a fh pointer is stored in ll_unclosed_fh_set only if a file was opened using Client::ll_open or Client::ll_create. This is fine but let's think of a case where the client opens a file with the high level APIs like Client::open or Client::openat (which makes uses of Client::create_and_open which stores the fh pointer in fd_map); fetches the file handle and uses the retrieved pointer to feed into the low level API like Client::ll_writev and since ll_unclosed_fh_set has no fh in it(because file wasn't opened with low level APIs), we have a trouble here. Does this case(of using high level and low level APIs together) hold any ground @vshankar @mchangir ?

Are you mixing using the high and low level calls? If yes, that need to be avoided - you either use the high level or the low level, isn't it @dparmar18 ?

vshankar · 2024-02-20T11:37:02Z

a fh pointer is stored in ll_unclosed_fh_set only if a file was opened using Client::ll_open or Client::ll_create. This is fine but let's think of a case where the client opens a file with the high level APIs like Client::open or Client::openat (which makes uses of Client::create_and_open which stores the fh pointer in fd_map); fetches the file handle and uses the retrieved pointer to feed into the low level API like Client::ll_writev and since ll_unclosed_fh_set has no fh in it(because file wasn't opened with low level APIs), we have a trouble here. Does this case(of using high level and low level APIs together) hold any ground @vshankar @mchangir ?

Are you mixing using the high and low level calls? If yes, that need to be avoided - you either use the high level or the low level, isn't it @dparmar18 ?

Ethically yes, but if someone decides to play around with it, the above case prevails. Theoretically we should factor this in, no?

Is someone is shooting themselves in the foot, we can only do so much, isn't it?

vshankar · 2024-02-20T11:44:12Z

@dparmar18 I'm now totally confused as what this change is aiming to fix - it was much clear earlier, sorry!

dparmar18 · 2024-02-20T11:49:17Z

a fh pointer is stored in ll_unclosed_fh_set only if a file was opened using Client::ll_open or Client::ll_create. This is fine but let's think of a case where the client opens a file with the high level APIs like Client::open or Client::openat (which makes uses of Client::create_and_open which stores the fh pointer in fd_map); fetches the file handle and uses the retrieved pointer to feed into the low level API like Client::ll_writev and since ll_unclosed_fh_set has no fh in it(because file wasn't opened with low level APIs), we have a trouble here. Does this case(of using high level and low level APIs together) hold any ground @vshankar @mchangir ?

Are you mixing using the high and low level calls? If yes, that need to be avoided - you either use the high level or the low level, isn't it @dparmar18 ?

Ethically yes, but if someone decides to play around with it, the above case prevails. Theoretically we should factor this in, no?

Is someone is shooting themselves in the foot, we can only do so much, isn't it?

Well at least they can be warned, but if this doesn't make sense then I'll switch to calling the helper from the low level APIs since this helper can't be used in Client::_preadv_pwritev_locked because it is used by both high and low level APIs

dparmar18 · 2024-02-20T11:55:33Z

@dparmar18 I'm now totally confused as what this change is aiming to fix - it was much clear earlier, sorry!

yeah since it is incomplete, it is bound to create confusions, wait a bit im pushing the changes since i know the answer for #55457 (comment)

It is named _ll_fh_exists meaning it is a helper func only to be used with low level APIs. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

Along with some minor adjustments to the code to make use of same int for all the ops. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

and guard the `if (!mref_reader.is_state_satisfied())` stmt with braces. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2024-02-20T12:43:12Z

[----------] 25 tests from TestClient (7927 ms total)

[----------] Global test environment tear-down
[==========] 25 tests from 1 test suite ran. (7927 ms total)
[  PASSED  ] 25 tests.

src/client/Client.cc

dparmar18 · 2024-02-22T11:08:33Z

jenkins test api

vshankar · 2024-02-27T04:36:34Z

https://pulpito.ceph.com/?branch=wip-vshankar-testing-20240226.124304

vshankar · 2024-02-27T05:59:39Z

https://pulpito.ceph.com/vshankar-2024-02-27_04:05:06-fs-wip-vshankar-testing-20240226.124304-testing-default-smithi/

* refs/pull/55457/head: client: check for bad file handle in low level I/O APIs client: check for bad file handle in ll_preadv_pwritev client: add function to check if file handle exists src/test: test async I/O with invalid/closed file handle

vshankar · 2024-03-04T10:15:43Z

Reruning tests with #55909 and #55908 included. See - https://pulpito.ceph.com/vshankar-2024-03-04_08:26:39-fs-wip-vshankar-testing-20240304.042522-testing-default-smithi/

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#5th-March-2024

src/test/client/nonblocking.cc

github-actions bot added cephfs Ceph File System tests labels Feb 6, 2024

dparmar18 force-pushed the wip-64313 branch from 15ec5a5 to a149a84 Compare February 6, 2024 07:58

dparmar18 requested a review from a team February 6, 2024 07:59

dparmar18 force-pushed the wip-64313 branch from a149a84 to 8f8576a Compare February 6, 2024 08:11

dparmar18 requested a review from vshankar February 6, 2024 08:12

dparmar18 added the needs-review label Feb 6, 2024

dparmar18 force-pushed the wip-64313 branch from 8f8576a to b968ad6 Compare February 6, 2024 08:30

mchangir reviewed Feb 8, 2024

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-64313 branch from b968ad6 to a040f70 Compare February 9, 2024 11:56

mchangir reviewed Feb 9, 2024

View reviewed changes

src/test/client/nonblocking.cc Outdated Show resolved Hide resolved

src/test/client/nonblocking.cc Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-64313 branch 2 times, most recently from f6da755 to 188c5d3 Compare February 9, 2024 13:26

vshankar requested a review from a team February 14, 2024 05:25

vshankar reviewed Feb 14, 2024

View reviewed changes

github-actions bot added the needs-rebase label Feb 14, 2024

src/test: test async I/O with invalid/closed file handle

441dbb8

Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the wip-64313 branch from 188c5d3 to 2f843c0 Compare February 17, 2024 07:50

github-actions bot removed the needs-rebase label Feb 17, 2024

dparmar18 force-pushed the wip-64313 branch 3 times, most recently from 66d093a to 8b6230c Compare February 17, 2024 08:42

client: add function to check if file handle exists

5d1255c

It is named _ll_fh_exists meaning it is a helper func only to be used with low level APIs. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the wip-64313 branch 2 times, most recently from 24c962a to 6a23327 Compare February 20, 2024 12:19

client: check for bad file handle in ll_preadv_pwritev

5099c4e

Along with some minor adjustments to the code to make use of same int for all the ops. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the wip-64313 branch from 6a23327 to da8435c Compare February 20, 2024 12:28

client: check for bad file handle in low level I/O APIs

6614933

and guard the `if (!mref_reader.is_state_satisfied())` stmt with braces. Fixes: https://tracker.ceph.com/issues/64313 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the wip-64313 branch from da8435c to 6614933 Compare February 20, 2024 12:40

dparmar18 requested review from mchangir and vshankar February 20, 2024 12:42

vshankar reviewed Feb 21, 2024

View reviewed changes

src/client/Client.cc Show resolved Hide resolved

vshankar added the wip-vshankar-testing3 label Feb 26, 2024

vshankar approved these changes Mar 6, 2024

View reviewed changes

vshankar merged commit 9fd7ad5 into ceph:main Mar 6, 2024

vshankar removed the wip-vshankar-testing3 label Mar 6, 2024

dparmar18 mentioned this pull request Mar 6, 2024

squid: client: do not proceed with I/O if filehandle is invalid #55997

Merged

14 tasks

vshankar reviewed Mar 6, 2024

View reviewed changes

src/test/client/nonblocking.cc Show resolved Hide resolved

vshankar mentioned this pull request Mar 21, 2024

cephfs_mirror, qa: fix test failure test_cephfs_mirror_cancel_mirroring_and_readd #56118

Merged

14 tasks

dparmar18 mentioned this pull request May 20, 2024

src/test: add testcase file for low level sync io code paths #57576

Merged

14 tasks

This was referenced Jul 2, 2024

reef: client: do not proceed with I/O if filehandle is invalid #58397

Merged

quincy: client: do not proceed with I/O if filehandle is invalid #58398

Closed

Conversation

dparmar18 commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dparmar18 commented Feb 9, 2024

Uh oh!

mchangir commented Feb 10, 2024

Uh oh!

dparmar18 commented Feb 12, 2024

Uh oh!

mchangir commented Feb 12, 2024

Uh oh!

dparmar18 commented Feb 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dparmar18 Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 14, 2024

Uh oh!

dparmar18 commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Feb 20, 2024

Uh oh!

vshankar commented Feb 20, 2024

Uh oh!

vshankar commented Feb 20, 2024

Uh oh!

dparmar18 commented Feb 20, 2024

Uh oh!

dparmar18 commented Feb 20, 2024

Uh oh!

dparmar18 commented Feb 20, 2024

Uh oh!

Uh oh!

dparmar18 commented Feb 22, 2024

Uh oh!

vshankar commented Feb 27, 2024

Uh oh!

vshankar commented Feb 27, 2024

Uh oh!

vshankar commented Mar 4, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dparmar18 commented Feb 6, 2024 •

edited

Loading

dparmar18 Feb 20, 2024 •

edited

Loading

dparmar18 commented Feb 17, 2024 •

edited

Loading