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

FileJournal:_fdump wrongly returns if journal is currently unreadable. #6406

Merged
merged 1 commit into from Nov 2, 2015

Conversation

Projects
None yet
3 participants
@xiexingguo
Copy link
Member

xiexingguo commented Oct 28, 2015

_fdump wrongly returns if journal is currently unreadable, and fix some other errors also.
Fixes: #13626
Signed-off-by: xie xingguo xie.xingguo@zte.com.cn

@ghost ghost added bug fix core labels Oct 28, 2015

@ghost ghost self-assigned this Oct 28, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 28, 2015

After rados a rados suite run

Reviewed-by: Loic Dachary <ldachary@redhat.com>

@ghost ghost added the needs-qa label Oct 28, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 28, 2015

@xiexingguo Nice catch ! Are you willing to run a rados suite ?

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Oct 28, 2015

@dachary I'll be glad to, but I currently don't know how. So perhaps next time when I am ready?

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 28, 2015

@xiexingguo if you have access to an OpenStack cluster, you can follow the instructions at https://github.com/dachary/teuthology/tree/openstack#openstack-backend and then run

teuthology-openstack --verbose --key-name myself --key-filename ~/Downloads/myself --suite rados/singleton --subset $(expr $RANDOM % 18)/18 --suite-branch master --ceph xxg-wip-13626 --ceph-git-url https://github.com/xiexingguo/ceph.git

If you do not have access to an OpenStack cluster, I can provide you one.

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Oct 28, 2015

@dachary I don't. Please provide me one and I'll try to get everything done.

// We'd better quit in this situation, otherwise
// this fd is going to be replaced and therefore
// has no more choice to be correctly closed.
return -err;

This comment has been minimized.

@dzafman

dzafman Oct 28, 2015

Member

Looking at the close(2) man page there are only 3 possible errors:

  1. EBADF (not a valid file descriptor) This could be a corrupt fd value. No point in not going forward.
  2. EINTR (interrupted by signal) The TEMP_FAILURE_RETRY() macro retries as long as error is EINTR.
  3. EIO (I/O error) This happens because an async write() didn't report the error, but the fd is still closed afterwards.

This change should be removed.

@@ -629,7 +633,7 @@ int FileJournal::_fdump(Formatter &f, bool simple)

if (!pos) {
dout(2) << "_dump -- not readable" << dendl;
return false;
break;

This comment has been minimized.

@dzafman

dzafman Oct 28, 2015

Member

Let's "err = -EINVAL;" before the break.

This comment has been minimized.

@xiexingguo

xiexingguo Oct 28, 2015

Author Member

I didn't consider it as an error here. But it's OK to do so.

FileJournal:_fdump wrongly returns if journal is currently unreadable.
_fdump wrongly returns if journal is currently unreadable, and fix some other errors also.
Fixes: #13626
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Oct 28, 2015

@dzafman Fix up as you suggested.

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Oct 29, 2015

@dzafman Hi, david.
I am sorry that I compulsively delete the original repository. Do I need to close this pull request and re-pull? Or it just can be correctly processed anyway.

Sorry for the inconvenience:-C

@dzafman

This comment has been minimized.

Copy link
Member

dzafman commented Oct 29, 2015

@xiexingguo It looks like it is ok. I fetched your branch into my repo if anything goes wrong.

@xiexingguo

This comment has been minimized.

Copy link
Member Author

xiexingguo commented Nov 2, 2015

@dzafman Hi, David.
Is there any problem about this commit? I see this commit has been hung for a couple of days.
Sorry for disturbing :)

@dzafman

This comment has been minimized.

Copy link
Member

dzafman commented Nov 2, 2015

@xiexingguo Yes, it did pass my tests. It slipped my mind that it was part of it.

dzafman added a commit that referenced this pull request Nov 2, 2015

Merge pull request #6406 from xiexingguo/xxg-wip-13626
FileJournal:_fdump wrongly returns if journal is currently unreadable.

Reviewed-by: Loic Dachary <ldachary@redhat.com>
Reviewed-by: David Zafman <dzafman@redhat.com>

@dzafman dzafman merged commit efe165e into ceph:master Nov 2, 2015

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.