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

rgw: set object accounted size correctly #14950

Merged
merged 1 commit into from Jun 8, 2017

Conversation

Projects
None yet
4 participants
@fangyuxiangGL
Contributor

fangyuxiangGL commented May 4, 2017

sometimes, object accounted size is set wrong,
because we don't know the object size if don't resort to the compression info or manifest without object index entry.
e.g, when i use s3cmd do copy object(bucket_A/obj_A -> bucket_B/obj_B, assume the size of obj_A is 4M).
then i use s3cmd do list bucket, I got obj_B size is 512K, it is the head size apparently.

@liewegas liewegas requested a review from rzarzynski May 4, 2017

@@ -8741,6 +8741,8 @@ int RGWRados::get_obj_state_impl(RGWObjectCtx *rctx, const RGWBucketInfo& bucket
s->manifest.set_head(bucket_info.placement_rule, obj, s->size); /* patch manifest to reflect the head we just read, some manifests might be
broken due to old bugs */
s->size = s->manifest.get_obj_size();
if (s->accounted_size < s->size)

This comment has been minimized.

@rzarzynski

rzarzynski May 4, 2017

Contributor

Why do we need the check? Shouldn't we always report the accounted_size as the result of get_obj_size()?

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL May 5, 2017

Contributor

assume that compression is enabled, get_obj_size() will less than the original size.

This comment has been minimized.

@cbodley

cbodley May 8, 2017

Contributor

this is making sense to me now. accounted_size defaults to size above, before looking at the RGWCompressionInfo. if we update size here, that update should also be applied to accounted_size as well

things are tricky if -both- accounted_size is set from RGWCompressionInfo -and- size is set based on the manifest. while it's possible for compression to increase the object size, i think it's safe to report to smaller of the two in this unlikely case

This comment has been minimized.

@cbodley

cbodley May 16, 2017

Contributor

you're right that this needs to be conditional, but i'd like to be more explicit about the condition being if (!compressed). can you add a flag above for that? something like:

   s->accounted_size = s->size;
 
   auto iter = s->attrset.find(RGW_ATTR_COMPRESSION);
-  if (iter != s->attrset.end()) {
+  const bool compressed = iter != s->attrset.end();
+  if (compressed) {
     // use uncompressed size for accounted_size
     try {

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL May 16, 2017

Contributor

ok, no problem

This comment has been minimized.

@cbodley

cbodley May 16, 2017

Contributor

thanks for adding the compressed flag - can you update this check to say if (!compressed)?

This comment has been minimized.

@cbodley

cbodley May 23, 2017

Contributor

please change this check from if (s->accounted_size < s->size) to if (!compressed)

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 15, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 15, 2017

@fangyuxiangGL i haven't been able to reproduce this one. here's what i tried:

$ cd ~/ceph/build
$ OSD=1 MON=1 MDS=0 MGR=0 RGW=1 ../src/vstart.sh -n -d
$ bin/radosgw-admin -c ceph.conf zone placement modify --rgw-zone=default --placement-id=default-placement --compression=snappy
[restart radosgw]

$ s3cmd mb s3://BUCKET s3://BUCKET2
Bucket 's3://BUCKET/' created
Bucket 's3://BUCKET2/' created

$ s3cmd put big.iso s3://BUCKET/                                                                                                                
upload: 'big.iso' -> 's3://BUCKET/big.iso'  [part 1 of 69, 15MB] [1 of 1]
 15728640 of 15728640   100% in    2s     7.40 MB/s  done
...
upload: 'big.iso' -> 's3://BUCKET/big.iso'  [part 69 of 69, 4MB] [1 of 1]
 4194304 of 4194304   100% in    0s    97.11 MB/s  done

$ s3cmd ls s3://BUCKET/
2017-05-15 14:14 1073741824   s3://BUCKET/big.iso

$ s3cmd cp s3://BUCKET/big.iso s3://BUCKET2/
remote copy: 's3://BUCKET/big.iso' -> 's3://BUCKET2/big.iso'

$ s3cmd ls s3://BUCKET2/
2017-05-15 14:15 1073741824   s3://BUCKET2/big.iso

can you share your steps to reproduce this? we'll also want to create a tracker ticket for backport

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 16, 2017

@cbodley

I think circumstance as below should be meet if you want to reproduce it.

  1. BUCKET should not be configured with compress, so it won't get the right accounted size from compression info.

  2. BUCKET and BUCKET2 should be configured with same data pool, so RGWRados::copy_obj won't return early(e.g, copy_data is True and go into RGWRados::copy_obj_data, then return )

Frankly speaking, I found this bug in jewel, so it may be fixed in master, but I just read the code from master, i think this bug still exists.

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 16, 2017

@fangyuxiangGL okay thanks, it does reproduce when compression is off

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 16, 2017

one of your commits is changing the Beast submodule, can you undo that part?

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 17, 2017

retest this please

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 19, 2017

@cbodley

it's ok now..

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 19, 2017

thanks @fangyuxiangGL. your initial commit has if (s->accounted_size < s->size) - can you please change that part to if (!compressed) and squash the two commits?

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 23, 2017

@cbodley

done it..

// use uncompressed size for accounted_size
try {
RGWCompressionInfo info;
auto p = iter->second.begin();
::decode(info, p);
s->accounted_size = info.orig_size;
if (s->accounted_size < info.orig_size)
s->accounted_size = info.orig_size;

This comment has been minimized.

@cbodley

cbodley May 23, 2017

Contributor

this change isn't needed

This comment has been minimized.

@fangyuxiangGL

fangyuxiangGL May 24, 2017

Contributor

sorry, I misunderstood.. will do it..

@@ -8741,6 +8741,8 @@ int RGWRados::get_obj_state_impl(RGWObjectCtx *rctx, const RGWBucketInfo& bucket
s->manifest.set_head(bucket_info.placement_rule, obj, s->size); /* patch manifest to reflect the head we just read, some manifests might be
broken due to old bugs */
s->size = s->manifest.get_obj_size();
if (s->accounted_size < s->size)

This comment has been minimized.

@cbodley

cbodley May 23, 2017

Contributor

please change this check from if (s->accounted_size < s->size) to if (!compressed)

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 24, 2017

@cbodley

done..

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 24, 2017

@fangyuxiangGL thanks, looks good. i'd like to get this backported to kraken as well, could you please add this to your commit message? Fixes: http://tracker.ceph.com/issues/20071

rgw: set object accounted size correctly
sometimes, object accounted size is set wrong,
because we don't konw the object size if don't resort to the compression info or manifest.
e.g, when i use s3cmd do copy object(bucket_A/obj_A -> bucket_B/obj_B, assume the size of obj_A is 4M).
then i use s3cmd do list bucket, I got obj_B size is 512K, it is the head size apparently.

Fixes: http://tracker.ceph.com/issues/20071

Signed-off-by: fang yuxiang <fang.yuxiang@eisoo.com>
@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented May 25, 2017

@cbodley

added

@fangyuxiangGL

This comment has been minimized.

Contributor

fangyuxiangGL commented Jun 1, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 1, 2017

@fangyuxiangGL thanks, i'll include in my next teuthology testing branch. hopefully today

@cbodley

This comment has been minimized.

@cbodley cbodley merged commit 43cad44 into ceph:master Jun 8, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@fangyuxiangGL fangyuxiangGL deleted the fangyuxiangGL:set-obj-accounted-size branch Jan 10, 2018

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