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_file: fix misuse of make_key_name before make_fhk #15108

Merged
merged 1 commit into from Jun 24, 2017

Conversation

Projects
None yet
3 participants
@guihecheng
Contributor

guihecheng commented May 16, 2017

The make_fhk calls make_key_name internally.

Signed-off-by: Gui Hecheng guihecheng@cmss.chinamobile.com

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented May 16, 2017

@guihecheng does this fix an observed problem? I agree, this will lead to an incorrect fh_key value and should have some observed effect. ok, so no, it probably has no observed effect, but changing it alters the value of object keys on the domain; we don't currently make any use of the stored value (which would be out-of-sync after this change);

@mattbenjamin

this looks correct; I'm going to merge after some sanity checking

@mattbenjamin

mattbenjamin requested changes May 16, 2017 edited

ok, so we could only merge this change if we -at least- remove asserts on decoded key (from serialized ux_key1), and maybe need to revisit that, as it is not versioned; I think after discussing w/@cbodley that 1) because the decoder -is- versioned, we can make the assert conditional, and 2) we should then add logic for safely updating both metadata attributes affected (that should happen in a safe context, before returning from lookup_fh(), probably);

@guihecheng

This comment has been minimized.

Contributor

guihecheng commented May 17, 2017

@mattbenjamin oh, yes, I forgot the versioned decoder, so we shall have this after it is sate to update.

@guihecheng

This comment has been minimized.

Contributor

guihecheng commented Jun 2, 2017

@mattbenjamin This introduces 'version' for fh_key and assert is conditional, but I didn't touch lookup_fh() as commented for now, could you please explain a bit more ?

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Jun 6, 2017

@guihecheng iiuc, since this change alters the computed value of fh_key, an old value could have been stored, and we'd need to update it if we detect an old stored version

@guihecheng

This comment has been minimized.

Contributor

guihecheng commented Jun 8, 2017

@mattbenjamin yes, I agree that, old versioned 'unix-key1' attr should be updated.
So, could we just do an update right inside decode_attrs() ? it should be safe, because we call it always under rgw_fh->mtx.
If we update inside lookup_fh(), then the attr will get updated only upon a second lookup_fh(), because we need the first lookup_fh() to kick decode_attrs() to go.
Or if we do the version check in lookup_fh(), it would be a bit expensive.

Gui Hecheng
rgw_file: fix misuse of make_key_name before make_fhk
The make_fhk calls make_key_name internally.
Also, because this makes the stored fhk and the new constructed fhk
mismatch, we make the assertion check conditional based on versioned
fhk and bump up the version.

Also, we shall safely update the old_key using update_fhk().

Signed-off-by: Gui Hecheng <guihecheng@cmss.chinamobile.com>
@guihecheng

This comment has been minimized.

Contributor

guihecheng commented Jun 14, 2017

@mattbenjamin hi, I updated with my solution, it works for me together with this fix:#15679. please review this when free, thx~

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Jun 23, 2017

@guihecheng this looks good; I'll test the key upgrade case and if it works, will merge this

@mattbenjamin mattbenjamin self-assigned this Jun 23, 2017

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Jun 23, 2017

retest this please

@mattbenjamin

@guihecheng this seems to work great

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Jun 24, 2017

retest this please

@mattbenjamin mattbenjamin merged commit e97210e into ceph:master Jun 24, 2017

4 of 5 checks passed

default Build started sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details

@guihecheng guihecheng deleted the guihecheng:rgw_file-fix branch Jun 26, 2017

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