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

bluestore: wrap blob id when it reaches maximum value of int16_t #15654

Merged
merged 1 commit into from Jun 15, 2017

Conversation

Projects
None yet
5 participants
@lixiaoy1
Contributor

lixiaoy1 commented Jun 13, 2017

Blob id exceeds maximum value, this patch is to prevent it.
Fixes: http://tracker.ceph.com/issues/19555

Signed-off-by: Xiaoyan Li xiaoyan.li@intel.com

@tchaikov tchaikov changed the title from Blob id exceeds maximum value to bluestore: wrap blob id when it reaches maximum value of int16_t Jun 13, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Jun 13, 2017

Contributor

could you add a "Signed-off-by" line at the end of your commit message? "git commit -s" will do the trick for you. see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#1-sign-your-work, to be specific, you need to add angle brackets around your email address.

also, you might need to put "Fixes: http://tracker.ceph.com/issues/19555" right before your Signed-off-by line. normally, we don't use the notation of "Bug http://tracker.ceph.com/issues/19555".

Contributor

tchaikov commented Jun 13, 2017

could you add a "Signed-off-by" line at the end of your commit message? "git commit -s" will do the trick for you. see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#1-sign-your-work, to be specific, you need to add angle brackets around your email address.

also, you might need to put "Fixes: http://tracker.ceph.com/issues/19555" right before your Signed-off-by line. normally, we don't use the notation of "Bug http://tracker.ceph.com/issues/19555".

@tchaikov tchaikov requested a review from liewegas Jun 13, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Jun 13, 2017

Contributor

@lixiaoy1 did you forget to push the updated commit?

Contributor

tchaikov commented Jun 13, 2017

@lixiaoy1 did you forget to push the updated commit?

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@ceph-jenkins

This comment has been minimized.

Show comment
Hide comment
@ceph-jenkins

ceph-jenkins Jun 14, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

ceph-jenkins commented Jun 14, 2017

Can one of the admins verify this patch?

@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@liewegas I add the function allocate_spanning_blob_id(). And check bid not used when +1. Add assert(bid>=0) when using bid to assert when all id between 0-SHRT_MAX is used.

@tchaikov The commit message is updated.

Contributor

lixiaoy1 commented Jun 14, 2017

@liewegas I add the function allocate_spanning_blob_id(). And check bid not used when +1. Add assert(bid>=0) when using bid to assert when all id between 0-SHRT_MAX is used.

@tchaikov The commit message is updated.

@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@tchaikov small note: int16_t b = (SHRT_MAX+1); b is -32768, not -1.

Contributor

lixiaoy1 commented Jun 14, 2017

@tchaikov small note: int16_t b = (SHRT_MAX+1); b is -32768, not -1.

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 14, 2017

Member

the coding style is not very compatible, btw

Member

xiexingguo commented Jun 14, 2017

the coding style is not very compatible, btw

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Jun 14, 2017

Contributor

small note: int16_t b = (SHRT_MAX+1); b is -32768, not -1.

thanks for the reminder, i was wrong. yeah, it wraps to numeric_limits<int16_t>::min().

Contributor

tchaikov commented Jun 14, 2017

small note: int16_t b = (SHRT_MAX+1); b is -32768, not -1.

thanks for the reminder, i was wrong. yeah, it wraps to numeric_limits<int16_t>::min().

@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@tchaikov @xiexingguo Thank you for review, the patch is updated based on your comments.

Contributor

lixiaoy1 commented Jun 14, 2017

@tchaikov @xiexingguo Thank you for review, the patch is updated based on your comments.

Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
Show outdated Hide outdated src/os/bluestore/BlueStore.cc
@@ -2003,6 +2005,28 @@ void BlueStore::ExtentMap::update(KeyValueDB::Transaction t,
}
}
bid_t BlueStore::ExtentMap::allocate_spanning_blob_id()

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

nit, make this method a const

@tchaikov

tchaikov Jun 14, 2017

Contributor

nit, make this method a const

This comment has been minimized.

@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

The return variable is a right value, should we make it const? As with const it causes the warning "warning: type qualifiers ignored on function return type".

@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

The return variable is a right value, should we make it const? As with const it causes the warning "warning: type qualifiers ignored on function return type".

This comment has been minimized.

@xiexingguo

xiexingguo Jun 14, 2017

Member

The return variable is a right value, should we make it const?

I guess what does kefu mean is to make the method itself const, not the return value...

@xiexingguo

xiexingguo Jun 14, 2017

Member

The return variable is a right value, should we make it const?

I guess what does kefu mean is to make the method itself const, not the return value...

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

yeah, to be specific:

bid_t BlueStore::ExtentMap::allocate_spanning_blob_id() const
@tchaikov

tchaikov Jun 14, 2017

Contributor

yeah, to be specific:

bid_t BlueStore::ExtentMap::allocate_spanning_blob_id() const
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov Jun 14, 2017

Contributor

also, needs rebase.

Contributor

tchaikov commented Jun 14, 2017

also, needs rebase.

@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@tchaikov I think I updated all the points you referred and rebased. Thx.

Contributor

lixiaoy1 commented Jun 14, 2017

@tchaikov I think I updated all the points you referred and rebased. Thx.

@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@tchaikov Commit updated, please have a look. Thx.

Contributor

lixiaoy1 commented Jun 14, 2017

@tchaikov Commit updated, please have a look. Thx.

if (bid < 0) bid = 0;
}
} while (bid != begin_bid);
assert(0 == "no available blob id");

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

i am not sure this compiles... it's supposed to return something.

@tchaikov

tchaikov Jun 14, 2017

Contributor

i am not sure this compiles... it's supposed to return something.

This comment has been minimized.

@lixiaoy1

This comment has been minimized.

@tchaikov

tchaikov Jun 14, 2017

Contributor

@lixiaoy1 i am not questioning the assert() statement. i am wondering if a function is not returning void then the compiler would expect it to return a variable of the returning type.

@tchaikov

tchaikov Jun 14, 2017

Contributor

@lixiaoy1 i am not questioning the assert() statement. i am wondering if a function is not returning void then the compiler would expect it to return a variable of the returning type.

@xiexingguo

This comment has been minimized.

Show comment
Hide comment
@xiexingguo

xiexingguo Jun 14, 2017

Member

Warnings:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.cc:22:0:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.h:779:56: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
     const decltype(Blob::id) allocate_spanning_blob_id();
                                                        ^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.cc:2024:61: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
 const bid_t BlueStore::ExtentMap::allocate_spanning_blob_id()
Member

xiexingguo commented Jun 14, 2017

Warnings:

In file included from /home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.cc:22:0:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.h:779:56: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
     const decltype(Blob::id) allocate_spanning_blob_id();
                                                        ^
/home/jenkins-build/build/workspace/ceph-pull-requests/src/os/bluestore/BlueStore.cc:2024:61: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
 const bid_t BlueStore::ExtentMap::allocate_spanning_blob_id()
@lixiaoy1

This comment has been minimized.

Show comment
Hide comment
@lixiaoy1

lixiaoy1 Jun 14, 2017

Contributor

@xiexingguo The warnings are caused by const bid_t BlueStore::ExtentMap::allocate_spanning_blob_id(), the returned value doesn't need to be const. I removed it.

Contributor

lixiaoy1 commented Jun 14, 2017

@xiexingguo The warnings are caused by const bid_t BlueStore::ExtentMap::allocate_spanning_blob_id(), the returned value doesn't need to be const. I removed it.

@xiexingguo

lgtm

@tchaikov

lgtm aside from the nit.

bluestore: wrap blob id when it reaches maximum value of int16_t
Blob id exceeds maximum value, this patch is to prevent it.
Fixes: http://tracker.ceph.com/issues/19555

Signed-off-by: Xiaoyan Li <xiaoyan.li@intel.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Jun 15, 2017

Member

Thank you! 👍

Member

liewegas commented Jun 15, 2017

Thank you! 👍

@liewegas liewegas merged commit 9f574bc into ceph:master Jun 15, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@lixiaoy1 lixiaoy1 deleted the lixiaoy1:lisa_bid branch Jun 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment