From: Andrew Price <anprice@redhat.com>
To: Andreas Gruenbacher <agruenba@redhat.com>
Cc: gfs2@lists.linux.dev
Subject: Re: [PATCH] gfs2: Improve gfs2_consist_inode() usage
Date: Fri, 12 Jan 2024 10:10:44 +0000 [thread overview]
Message-ID: <a4aa8f4a-dd34-4a23-80c6-5f583d95845b@redhat.com> (raw)
In-Reply-To: <CAHc6FU7O81t4dvrLaLFVt6EjkiwUowQD+8gBg8bNfF7mXYEFkQ@mail.gmail.com>
On 12/01/2024 00:07, Andreas Gruenbacher wrote:
> Andy,
>
> On Thu, Jan 11, 2024 at 6:43 PM Andrew Price <anprice@redhat.com> wrote:
>> gfs2_consist_inode() logs an error message with the source file and line
>> number. When we jump before calling it, the line number becomes less
>> useful as it no longer relates to the source of the error. To aid
>> troubleshooting, replace the gotos with the gfs2_consist_inode() calls
>> so that the error messages are more informative.
>
> this looks acceptable to me.
:)
> gfs2_consist_inode() is used inconsistently, though: in some cases, it
> reports an original error; in other cases, the error has already been
> reported and gfs2_consist_inode() only provides additional details. I
> guess we can't have nice code.
I don't think additional details could hurt. Perhaps there's room for
more cleanups around this function though.
>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>> fs/gfs2/dir.c | 28 +++++++++++++++++-----------
>> fs/gfs2/glops.c | 34 ++++++++++++++++++++--------------
>> fs/gfs2/xattr.c | 28 ++++++++++++++++------------
>> 3 files changed, 53 insertions(+), 37 deletions(-)
>>
>> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
>> index 560e4624c09f2..47355c87efc3b 100644
>> --- a/fs/gfs2/dir.c
>> +++ b/fs/gfs2/dir.c
>> @@ -562,15 +562,18 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf,
>> int ret = 0;
>>
>> ret = gfs2_dirent_offset(GFS2_SB(inode), buf);
>> - if (ret < 0)
>> - goto consist_inode;
>> -
>> + if (ret < 0) {
>> + gfs2_consist_inode(GFS2_I(inode));
>> + return ERR_PTR(-EIO);
>> + }
>> offset = ret;
>> prev = NULL;
>> dent = buf + offset;
>> size = be16_to_cpu(dent->de_rec_len);
>> - if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size, len, 1))
>> - goto consist_inode;
>> + if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size, len, 1)) {
>> + gfs2_consist_inode(GFS2_I(inode));
>> + return ERR_PTR(-EIO);
>> + }
>> do {
>> ret = scan(dent, name, opaque);
>> if (ret)
>> @@ -582,8 +585,10 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf,
>> dent = buf + offset;
>> size = be16_to_cpu(dent->de_rec_len);
>> if (gfs2_check_dirent(GFS2_SB(inode), dent, offset, size,
>> - len, 0))
>> - goto consist_inode;
>> + len, 0)) {
>> + gfs2_consist_inode(GFS2_I(inode));
>> + return ERR_PTR(-EIO);
>> + }
>> } while(1);
>>
>> switch(ret) {
>> @@ -598,7 +603,6 @@ static struct gfs2_dirent *gfs2_dirent_scan(struct inode *inode, void *buf,
>> return ERR_PTR(ret);
>> }
>>
>> -consist_inode:
>
> The code below is now unreachable, so I'll remove it.
Good catch, thanks.
Andy
>
>> gfs2_consist_inode(GFS2_I(inode));
>> return ERR_PTR(-EIO);
>> }
>> @@ -609,14 +613,16 @@ static int dirent_check_reclen(struct gfs2_inode *dip,
>> const void *ptr = d;
>> u16 rec_len = be16_to_cpu(d->de_rec_len);
>>
>> - if (unlikely(rec_len < sizeof(struct gfs2_dirent)))
>> - goto broken;
>> + if (unlikely(rec_len < sizeof(struct gfs2_dirent))) {
>> + gfs2_consist_inode(dip);
>> + return -EIO;
>> + }
>> ptr += rec_len;
>> if (ptr < end_p)
>> return rec_len;
>> if (ptr == end_p)
>> return -ENOENT;
>> -broken:
>> +
>> gfs2_consist_inode(dip);
>> return -EIO;
>> }
>> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
>> index 45653cbc8a87d..ae8d4731907d1 100644
>> --- a/fs/gfs2/glops.c
>> +++ b/fs/gfs2/glops.c
>> @@ -409,10 +409,14 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
>> struct inode *inode = &ip->i_inode;
>> bool is_new = inode->i_state & I_NEW;
>>
>> - if (unlikely(ip->i_no_addr != be64_to_cpu(str->di_num.no_addr)))
>> - goto corrupt;
>> - if (unlikely(!is_new && inode_wrong_type(inode, mode)))
>> - goto corrupt;
>> + if (unlikely(ip->i_no_addr != be64_to_cpu(str->di_num.no_addr))) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> + if (unlikely(!is_new && inode_wrong_type(inode, mode))) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> ip->i_no_formal_ino = be64_to_cpu(str->di_num.no_formal_ino);
>> inode->i_mode = mode;
>> if (is_new) {
>> @@ -449,26 +453,28 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
>> /* i_diskflags and i_eattr must be set before gfs2_set_inode_flags() */
>> gfs2_set_inode_flags(inode);
>> height = be16_to_cpu(str->di_height);
>> - if (unlikely(height > sdp->sd_max_height))
>> - goto corrupt;
>> + if (unlikely(height > sdp->sd_max_height)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> ip->i_height = (u8)height;
>>
>> depth = be16_to_cpu(str->di_depth);
>> - if (unlikely(depth > GFS2_DIR_MAX_DEPTH))
>> - goto corrupt;
>> + if (unlikely(depth > GFS2_DIR_MAX_DEPTH)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> ip->i_depth = (u8)depth;
>> ip->i_entries = be32_to_cpu(str->di_entries);
>>
>> - if (gfs2_is_stuffed(ip) && inode->i_size > gfs2_max_stuffed_size(ip))
>> - goto corrupt;
>> -
>> + if (gfs2_is_stuffed(ip) && inode->i_size > gfs2_max_stuffed_size(ip)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> if (S_ISREG(inode->i_mode))
>> gfs2_set_aops(inode);
>>
>> return 0;
>> -corrupt:
>> - gfs2_consist_inode(ip);
>> - return -EIO;
>> }
>>
>> /**
>> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
>> index 8c96ba6230d1b..17ae5070a90e6 100644
>> --- a/fs/gfs2/xattr.c
>> +++ b/fs/gfs2/xattr.c
>> @@ -96,30 +96,34 @@ static int ea_foreach_i(struct gfs2_inode *ip, struct buffer_head *bh,
>> return -EIO;
>>
>> for (ea = GFS2_EA_BH2FIRST(bh);; prev = ea, ea = GFS2_EA2NEXT(ea)) {
>> - if (!GFS2_EA_REC_LEN(ea))
>> - goto fail;
>> + if (!GFS2_EA_REC_LEN(ea)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> if (!(bh->b_data <= (char *)ea && (char *)GFS2_EA2NEXT(ea) <=
>> - bh->b_data + bh->b_size))
>> - goto fail;
>> - if (!gfs2_eatype_valid(sdp, ea->ea_type))
>> - goto fail;
>> + bh->b_data + bh->b_size)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> + if (!gfs2_eatype_valid(sdp, ea->ea_type)) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> error = ea_call(ip, bh, ea, prev, data);
>> if (error)
>> return error;
>>
>> if (GFS2_EA_IS_LAST(ea)) {
>> if ((char *)GFS2_EA2NEXT(ea) !=
>> - bh->b_data + bh->b_size)
>> - goto fail;
>> + bh->b_data + bh->b_size) {
>> + gfs2_consist_inode(ip);
>> + return -EIO;
>> + }
>> break;
>> }
>> }
>>
>> return error;
>> -
>> -fail:
>> - gfs2_consist_inode(ip);
>> - return -EIO;
>> }
>>
>> static int ea_foreach(struct gfs2_inode *ip, ea_call_t ea_call, void *data)
>> --
>> 2.43.0
>>
>
> Thanks,
> Andreas
>
prev parent reply other threads:[~2024-01-12 10:10 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-11 17:42 [PATCH] gfs2: Improve gfs2_consist_inode() usage Andrew Price
2024-01-12 0:07 ` Andreas Gruenbacher
2024-01-12 10:10 ` Andrew Price [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a4aa8f4a-dd34-4a23-80c6-5f583d95845b@redhat.com \
--to=anprice@redhat.com \
--cc=agruenba@redhat.com \
--cc=gfs2@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).