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

Wip client fsync #4778

Merged
merged 4 commits into from Jun 5, 2015
Merged

Wip client fsync #4778

merged 4 commits into from Jun 5, 2015

Conversation

ukernel
Copy link
Contributor

@ukernel ukernel commented May 27, 2015

No description provided.

@ukernel ukernel added bug-fix cephfs Ceph File System labels May 27, 2015
@gregsfortytwo gregsfortytwo self-assigned this May 27, 2015
@jcsp
Copy link
Contributor

jcsp commented May 28, 2015

Looks sane, I will try writing an automated test today

@@ -3711,8 +3731,9 @@ int Client::mark_caps_flushing(Inode *in)
int flushing = in->dirty_caps;
assert(flushing);

if (flushing && !in->flushing_caps) {
if (!in->flushing_caps) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're just clearing out the flushing check here because we already asserted it above, or was there some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Fri, May 29, 2015 at 6:21 AM, Gregory Farnum notifications@github.com
wrote:

In src/client/Client.cc
#4778 (comment):

@@ -3711,8 +3731,9 @@ int Client::mark_caps_flushing(Inode *in)
int flushing = in->dirty_caps;
assert(flushing);

  • if (flushing && !in->flushing_caps) {
  • if (!in->flushing_caps) {

You're just clearing out the flushing check here because we already
asserted it above, or was there some other reason?

the assertion above is the reason I remove it

Reply to this email directly or view it on GitHub
https://github.com/ceph/ceph/pull/4778/files#r31283951.

@gregsfortytwo gregsfortytwo assigned jcsp and unassigned gregsfortytwo May 28, 2015
Current code always updates flushing_cap_seq when marking dirty
caps as flushing. If there are old flushing caps, updating
flushing_cap_seq confuses Client::wait_sync_caps(uint64_t), make
it think that the old caps have been flushed.

Signed-off-by: Yan, Zheng <zyan@redhat.com>
Client::_fsync() calls Client::wait_sync_caps(uint64_t), which
waits for all inodes' flush caps. It's suboptimal, this patch
makes Client::_fsync() wait for flushing caps which belong to
the fsync inode.

Signed-off-by: Yan, Zheng <zyan@redhat.com>
Signed-off-by: Yan, Zheng <zyan@redhat.com>
Client::flush_caps(Inode *, MetaSession *) does not start flushing
dirty caps. It only re-send caps message for caps that are already
being flushed.

Signed-off-by: Yan, Zheng <zyan@redhat.com>
@ukernel
Copy link
Contributor Author

ukernel commented May 29, 2015

updated. only removed the out-dated comment for wait_sync_caps()

@gregsfortytwo gregsfortytwo assigned gregsfortytwo and unassigned jcsp May 29, 2015
@gregsfortytwo
Copy link
Member

@jcsp's test: ceph/ceph-qa-suite#449

@gregsfortytwo
Copy link
Member

@gregsfortytwo gregsfortytwo merged commit b20ea43 into master Jun 5, 2015
@gregsfortytwo gregsfortytwo deleted the wip-client-fsync branch June 5, 2015 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants