On 2020/2/18 22:10, Ilya Dryomov wrote:
On Tue, Feb 18, 2020 at 2:51 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
On Mon, 2020-02-17 at 23:42 +0800, Xiubo Li wrote:
On 2020/2/17 23:27, Ilya Dryomov wrote:
On Mon, Feb 17, 2020 at 4:02 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
On 2020/2/17 22:52, Ilya Dryomov wrote:
On Mon, Feb 17, 2020 at 12:28 PM <xiubli@xxxxxxxxxx> wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>
For example, if dentry and inode is NULL, the log will be:
ceph: lookup result=000000007a1ca695
ceph: submit_request on 0000000041d5070e for inode 000000007a1ca695
The will be confusing without checking the corresponding code carefully.
Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
---
fs/ceph/dir.c | 2 +-
fs/ceph/mds_client.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index ffeaff5bf211..245a262ec198 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -798,7 +798,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
err = ceph_handle_snapdir(req, dentry, err);
dentry = ceph_finish_lookup(req, dentry, err);
ceph_mdsc_put_request(req); /* will dput(dentry) */
- dout("lookup result=%p\n", dentry);
+ dout("lookup result=%d\n", err);
return dentry;
}
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index b6aa357f7c61..e34f159d262b 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2772,7 +2772,11 @@ int ceph_mdsc_submit_request(struct ceph_mds_client *mdsc, struct inode *dir,
ceph_get_cap_refs(ceph_inode(req->r_old_dentry_dir),
CEPH_CAP_PIN);
- dout("submit_request on %p for inode %p\n", req, dir);
+ if (dir)
+ dout("submit_request on %p for inode %p\n", req, dir);
+ else
+ dout("submit_request on %p\n", req);
Hi Xiubo,
It's been this way for a couple of years now. There are a lot more
douts in libceph, ceph and rbd that are sometimes fed NULL pointers.
I don't think replacing them with conditionals is the way forward.
I honestly don't know what security concern is addressed by hashing
NULL pointers, but that is what we have... Ultimately, douts are just
for developers, and when you find yourself having to chase individual
pointers, you usually have a large enough piece of log to figure out
what the NULL hash is.
Hi Ilya
For the ceph_lookup(). The dentry will be NULL(when the directory exists
or -ENOENT) or ERR_PTR(-errno) in most cases here, it seems for the
rename case it will be the old dentry returned.
So today I was trying to debug and get some logs from it, the
000000007a1ca695 really confused me for a long time before I dig into
the source code.
I was reacting to ceph_mdsc_submit_request() hunk. Feel free to tweak
ceph_lookup() or refactor it so that err is not threaded through three
different functions as Jeff suggested.
Hi Ilya
Oh okay. You are right we can figure out what we need via many other
dout logs.
I just saw some very confusing logs that the "dentry" in cpeh_lookup()
and the "inode" in _submit_ are all 000000007a1ca695, so I addressed
them both here.
Since Ilya objected to this patch, I'll drop it from testing for now.
Please send a v2 that addresses his concerns if you still want this in.
I have submitted a patch to fix printk to not obfuscate NULL and
error pointers and haven't heard any objections yet, so hopefully
this issue will become moot soon.
Hi Jeff, Ilya
Yeah, this is very cool for me. This will just work like the string
pointer formating, if it is NULL it then will be formated as
"null"/"nil" or something instead.
Thanks
BRs
Thanks,
Ilya
