Re: [PATCH] ceph: decoding error in ceph_update_snap_realm should return -EIO

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2021-06-03 at 15:57 +0200, Ilya Dryomov wrote:
> On Thu, Jun 3, 2021 at 3:39 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > Currently ceph_update_snap_realm returns -EINVAL when it hits a decoding
> > error, which is the wrong error code. -EINVAL implies that the user gave
> > us a bogus argument to a syscall or something similar. -EIO is more
> > descriptive when we hit a decoding error.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/snap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > index d07c1c6ac8fb..f8cac2abab3f 100644
> > --- a/fs/ceph/snap.c
> > +++ b/fs/ceph/snap.c
> > @@ -807,7 +807,7 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
> >         return 0;
> > 
> >  bad:
> > -       err = -EINVAL;
> > +       err = -EIO;
> >  fail:
> >         if (realm && !IS_ERR(realm))
> >                 ceph_put_snap_realm(mdsc, realm);
> 
> Hi Jeff,
> 
> Is this error code propagated anywhere important?
> 
> The vast majority of functions that have something to do with decoding
> use EINVAL as a default (usually out-of-bounds) error.  I agree that it
> is totally ambiguous, but EIO doesn't seem to be any better to me.  If
> there is a desire to separate these errors, I think we need to pick
> something much more distinctive.
> 

When I see EINVAL, I automatically wonder what bogus argument I passed
in somewhere, so I find that particularly deceptive here where the bug
is either from the MDS or we had some sort of low-level socket handling
problem.

OTOH, you have a good point. The callers universally ignore the error
code from this function. Perhaps we ought to just log a pr_warn message
or something if the decoding fails here instead?
-- 
Jeff Layton <jlayton@kernel.org>