On Tue, Jun 13, 2023 at 11:51 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote: > > > On 6/13/23 17:07, Ilya Dryomov wrote: > > On Mon, Jun 12, 2023 at 1:46 PM <xiubli@xxxxxxxxxx> wrote: > >> From: Xiubo Li <xiubli@xxxxxxxxxx> > >> > >> Multiple cephfs mounts on a host is increasingly common so disambiguating > >> messages like this is necessary and will make it easier to debug > >> issues. > >> > >> URL: https://tracker.ceph.com/issues/61590 > >> Cc: Patrick Donnelly <pdonnell@xxxxxxxxxx> > >> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx> > >> --- > >> fs/ceph/acl.c | 6 +- > >> fs/ceph/addr.c | 300 ++++++++++-------- > >> fs/ceph/caps.c | 709 ++++++++++++++++++++++++------------------- > >> fs/ceph/crypto.c | 45 ++- > >> fs/ceph/debugfs.c | 4 +- > >> fs/ceph/dir.c | 222 +++++++++----- > >> fs/ceph/export.c | 39 ++- > >> fs/ceph/file.c | 268 +++++++++------- > >> fs/ceph/inode.c | 528 ++++++++++++++++++-------------- > >> fs/ceph/ioctl.c | 10 +- > >> fs/ceph/locks.c | 62 ++-- > >> fs/ceph/mds_client.c | 616 +++++++++++++++++++++---------------- > >> fs/ceph/mdsmap.c | 25 +- > >> fs/ceph/metric.c | 5 +- > >> fs/ceph/quota.c | 31 +- > >> fs/ceph/snap.c | 186 +++++++----- > >> fs/ceph/super.c | 64 ++-- > >> fs/ceph/xattr.c | 97 +++--- > >> 18 files changed, 1887 insertions(+), 1330 deletions(-) > >> > >> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > >> index 8a56f979c7cb..970acd07908d 100644 > >> --- a/fs/ceph/acl.c > >> +++ b/fs/ceph/acl.c > >> @@ -15,6 +15,7 @@ > >> #include <linux/slab.h> > >> > >> #include "super.h" > >> +#include "mds_client.h" > >> > >> static inline void ceph_set_cached_acl(struct inode *inode, > >> int type, struct posix_acl *acl) > >> @@ -31,6 +32,7 @@ static inline void ceph_set_cached_acl(struct inode *inode, > >> > >> struct posix_acl *ceph_get_acl(struct inode *inode, int type, bool rcu) > >> { > >> + struct ceph_client *cl = ceph_inode_to_client(inode); > >> int size; > >> unsigned int retry_cnt = 0; > >> const char *name; > >> @@ -72,8 +74,8 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type, bool rcu) > >> } else if (size == -ENODATA || size == 0) { > >> acl = NULL; > >> } else { > >> - pr_err_ratelimited("get acl %llx.%llx failed, err=%d\n", > >> - ceph_vinop(inode), size); > >> + pr_err_ratelimited_client(cl, "%s %llx.%llx failed, err=%d\n", > >> + __func__, ceph_vinop(inode), size); > >> acl = ERR_PTR(-EIO); > >> } > >> > >> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c > >> index e62318b3e13d..c772639dc0cb 100644 > >> --- a/fs/ceph/addr.c > >> +++ b/fs/ceph/addr.c > >> @@ -79,18 +79,18 @@ static inline struct ceph_snap_context *page_snap_context(struct page *page) > >> */ > >> static bool ceph_dirty_folio(struct address_space *mapping, struct folio *folio) > >> { > >> - struct inode *inode; > >> + struct inode *inode = mapping->host; > >> + struct ceph_client *cl = ceph_inode_to_client(inode); > >> struct ceph_inode_info *ci; > >> struct ceph_snap_context *snapc; > >> > >> if (folio_test_dirty(folio)) { > >> - dout("%p dirty_folio %p idx %lu -- already dirty\n", > >> - mapping->host, folio, folio->index); > >> + dout_client(cl, "%s %llx.%llx %p idx %lu -- already dirty\n", > >> + __func__, ceph_vinop(inode), folio, folio->index); > > While having context information attached to each dout is nice, it > > certainly comes at a price of a lot of churn and automated backport > > disruption. > > Yeah, certainly this will break automated backporting. But this should > be okay, I can generate the backport patches for each stable release for > this patch series, so after this it will make the automated backporting > work. > > > I wonder how much value doing this for douts as opposed > > to just pr_* messages actually brings? > > I think the 'dout()' was introduced by printing more context info, which > includes module/function names and line#, when the > CONFIG_CEPH_LIB_PRETTYDEBUG is enabled. dout() is just a wrapper around pr_debug(). It doesn't have anything to do with CONFIG_CEPH_LIB_PRETTYDEBUG per se which adds file names and line numbers. IIRC dout() by itself just adds a space at the front to make debugging spew stand out. > > Maybe we can remove CONFIG_CEPH_LIB_PRETTYDEBUG now, since the pr_* will > print the module name and also the caller for dout() and pr_* will print > the function name mostly ? To the best of my knowledge, CONFIG_CEPH_LIB_PRETTYDEBUG has always been disabled by default and it's not enabled by any distribution in their kernels. I don't think anyone out there would miss it, but then it's not hurting either -- it's less than 20 lines of code with all ifdef-ery included. Thanks, Ilya
