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

os/bluestore: move assert of read/write to base class #17033

Merged
merged 1 commit into from Aug 24, 2017

Conversation

Projects
None yet
4 participants
@mychoxin
Copy link
Contributor

commented Aug 15, 2017

Signed-off-by: mychoxin mychoxin@gmail.com

@mychoxin mychoxin force-pushed the mychoxin:yuanxin_put_assert_to_base branch from 119f66e to f29f334 Aug 15, 2017

@joscollin joscollin self-requested a review Aug 15, 2017

@joscollin

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

Jenkins retest this please

assert(len > 0);
assert(off < size);
assert(off + len <= size);
assert(is_valid_io(off, len));

This comment has been minimized.

Copy link
@joscollin

joscollin Aug 15, 2017

Member

It is different here. Only 3 assert statements

This comment has been minimized.

Copy link
@mychoxin

mychoxin Aug 16, 2017

Author Contributor

@majianpeng For this place, I think the original code misses 2 assert, do you agree?

This comment has been minimized.

Copy link
@majianpeng

majianpeng Aug 16, 2017

Member

Yes, PMEMDevice is based on byte address. So offset && length are not limited by block_size (in fact by directIO requirement)

assert(len > 0);
assert(off < size);
assert(off + len <= size);
assert(is_valid_io(off, len));

This comment has been minimized.

Copy link
@joscollin
assert(off < size);
assert(off + len <= size);
dout(5) << __func__ << " " << off << "~" << len << dendl;
assert(is_valid_io(off, len));

This comment has been minimized.

Copy link
@joscollin
@mychoxin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

assert(len > 0);
assert(off < size);
assert(off + len <= size);
assert(is_valid_io(off, len));

This comment has been minimized.

Copy link
@majianpeng

majianpeng Aug 16, 2017

Member

Yes, PMEMDevice is based on byte address. So offset && length are not limited by block_size (in fact by directIO requirement)

@mychoxin mychoxin force-pushed the mychoxin:yuanxin_put_assert_to_base branch 2 times, most recently from cacd4d9 to 8016be2 Aug 17, 2017

@mychoxin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2017

@joscollin I modified it, please take a look.

@joscollin
Copy link
Member

left a comment

LGTM

@@ -148,6 +148,13 @@ class BlockDevice {
virtual int invalidate_cache(uint64_t off, uint64_t len) = 0;
virtual int open(const std::string& path) = 0;
virtual void close() = 0;
virtual bool is_valid_io(uint64_t off, uint64_t len) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 19, 2017

Contributor

no need to make this a virtual method. a static method would suffice.

This comment has been minimized.

Copy link
@joscollin

joscollin Aug 19, 2017

Member

Right. is_valid_io is not getting called via an interface to be virtual. A simple overriding in PMEMDevice works well here.

@@ -148,6 +148,13 @@ class BlockDevice {
virtual int invalidate_cache(uint64_t off, uint64_t len) = 0;
virtual int open(const std::string& path) = 0;
virtual void close() = 0;
virtual bool is_valid_io(uint64_t off, uint64_t len) {

This comment has been minimized.

Copy link
@joscollin

joscollin Aug 19, 2017

Member

Right. is_valid_io is not getting called via an interface to be virtual. A simple overriding in PMEMDevice works well here.

@mychoxin mychoxin force-pushed the mychoxin:yuanxin_put_assert_to_base branch from 8016be2 to 60319f1 Aug 23, 2017

@mychoxin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

@tchaikov @joscollin Inoder to access the class member variable, I just remove 'virtual' not change it to static function, what do you think of it?

@@ -148,6 +148,13 @@ class BlockDevice {
virtual int invalidate_cache(uint64_t off, uint64_t len) = 0;
virtual int open(const std::string& path) = 0;
virtual void close() = 0;
bool is_valid_io(uint64_t off, uint64_t len) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 23, 2017

Contributor

mark this method private and const. and you can parenthesizes the condition expressions like

return (off % block_size == 0 &&
        len % block_size == 0 && 
 ...
        off + len <= size);

This comment has been minimized.

Copy link
@mychoxin

mychoxin Aug 23, 2017

Author Contributor

done, BlockDevice::is_valid_io is called by derived class, so I put it under protected. please have a look.

@@ -60,6 +60,11 @@ class PMEMDevice : public BlockDevice {
int invalidate_cache(uint64_t off, uint64_t len) override;
int open(const std::string &path) override;
void close() override;
bool is_valid_io(uint64_t off, uint64_t len) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 23, 2017

Contributor

the same applies.

os/bluestore: move assert of read/write to base class
Signed-off-by: mychoxin <mychoxin@gmail.com>

@mychoxin mychoxin force-pushed the mychoxin:yuanxin_put_assert_to_base branch from 60319f1 to dfa8847 Aug 23, 2017

comments addressed

@tchaikov tchaikov merged commit 8ed5fe0 into ceph:master Aug 24, 2017

4 of 5 checks passed

make check make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check (arm64) make check succeeded
Details
@joscollin

This comment has been minimized.

Copy link
Member

commented Aug 25, 2017

@mychoxin: Sorry, I was on leave. Your changes LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.