On Wed, 2019-07-03 at 12:01 -0400, Jeff Layton wrote: > On Wed, 2019-07-03 at 20:44 +0800, Yan, Zheng wrote: > > This series add support for auto reconnect after blacklisted. > > > > Auto reconnect is controlled by recover_session=<clean|no> mount option. > > Clean mode is enabled by default. In this mode, client drops dirty date > > and dirty metadata, All writable file handles are invalidated. Read-only > > file handles continue to work and caches are dropped if necessary. > > If an inode contains any lost file lock, read and write are not allowed. > > until all lost file locks are released. > > Just giving this a quick glance: > > Based on the last email discussion about this, I thought that you were > going to provide a mount option that someone could enable that would > basically allow the client to "soldier on" in the face of being > blacklisted and then unblacklisted, without needing to remount anything. > > This set seems to keep the force_reconnect option (patch #7) though, so > I'm quite confused at this point. What exactly is the goal of here? > s/of here/of this/ > There's also nothing in the changelogs or comments about > recover_session=brute, which seems like it ought to at least be > mentioned. > > At this point, I'm going to say NAK on this set until there is some > accompanying documentation about how you intend for this be used and by > whom. A patch for the mount.ceph(8) manpage would be a good place to > start. > So to be clear, I'd suggest (at a minimum): * writing an RFC patch for the mount.ceph manpage that explains what these options are and the expected behavior when they are used. In fact, I'd start with this before you do any more work on the code. We need to agree on the user-facing interface before anything else. * consider dropping the force_reconnect option. It seems like this shouldn't be needed if you've mounted with recover_session=brute. If you do decide to keep it, then this also needs to be clearly documented, or someone will attempt to use this to unwedge their client at some point without fully understanding the ramifications. * the cover letter should have a clear explanation of what this patchset is intended to do, and why you're adding it. Yes, I know we've discussed this on the list before, but in 5 years when someone new is revisiting the behavior and we've all moved on to do other things and paged this out of our brains, we want them to be able to find that rationale, and the associated discussion around it by searching the mailing list archives. * I'll also note that this is going to cause a behavior change. Clients that have been blacklisted and then unblacklisted are going to see different behavior if they're running on a kernel with this set. That may be expected and what we want, but it should be clearly stated in the changelog of the patch that makes this change. The manpage should probably have a nice detailed section on all of this as well, so that we can explain the expected behavior on various kernel versions. > > Yan, Zheng (9): > > libceph: add function that reset client's entity addr > > libceph: add function that clears osd client's abort_err > > ceph: allow closing session in restarting/reconnect state > > ceph: track and report error of async metadata operation > > ceph: pass filp to ceph_get_caps() > > ceph: return -EIO if read/write against filp that lost file locks > > ceph: add 'force_reconnect' option for remount > > ceph: invalidate all write mode filp after reconnect > > ceph: auto reconnect after blacklisted > > > > fs/ceph/addr.c | 30 +++++++---- > > fs/ceph/caps.c | 84 ++++++++++++++++++++---------- > > fs/ceph/file.c | 50 ++++++++++-------- > > fs/ceph/inode.c | 2 + > > fs/ceph/locks.c | 8 ++- > > fs/ceph/mds_client.c | 92 ++++++++++++++++++++++++++------- > > fs/ceph/mds_client.h | 6 +-- > > fs/ceph/super.c | 91 ++++++++++++++++++++++++++++++-- > > fs/ceph/super.h | 23 +++++++-- > > include/linux/ceph/libceph.h | 1 + > > include/linux/ceph/messenger.h | 1 + > > include/linux/ceph/mon_client.h | 1 + > > include/linux/ceph/osd_client.h | 2 + > > net/ceph/ceph_common.c | 38 +++++++++----- > > net/ceph/messenger.c | 5 ++ > > net/ceph/mon_client.c | 7 +++ > > net/ceph/osd_client.c | 24 +++++++++ > > 17 files changed, 365 insertions(+), 100 deletions(-) > > -- Jeff Layton <jlayton@xxxxxxxxxx>
