From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huawei.com>
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu,
adilger.kernel@dilger.ca, jack@suse.cz, yukuai3@huawei.com,
yebin10@huawei.com
Subject: Re: [RFC PATCH] ext4: convert symlink external data block mapping to bdev
Date: Wed, 6 Apr 2022 19:17:15 +0200 [thread overview]
Message-ID: <20220406171715.35euuzocoe4ljepe@quack3.lan> (raw)
In-Reply-To: <20220406084503.1961686-1-yi.zhang@huawei.com>
On Wed 06-04-22 16:45:03, Zhang Yi wrote:
> Symlink's external data block is one kind of metadata block, and now
> that almost all ext4 metadata block's page cache (e.g. directory blocks,
> quota blocks...) belongs to bdev backing inode except the symlink. It
> is essentially worked in data=journal mode like other regular file's
> data block because probably in order to make it simple for generic VFS
> code handling symlinks or some other historical reasons, but the logic
> of creating external data block in ext4_symlink() is complicated. and it
> also make things confused if user do not want to let the filesystem
> worked in data=journal mode. This patch convert the final exceptional
> case and make things clean, move the mapping of the symlink's external
> data block to bdev like any other metadata block does.
>
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
> This RFC patch follow the talking of whether if we could unify the
> journal mode of ext4 metadata blocks[1], it stop using the data=journal
> mode for the final exception case of symlink's external data block. Any
> comments are welcome, thanks.
>
> [1]. https://lore.kernel.org/linux-ext4/20220321151141.hypnhr6o4vng2sa6@quack3.lan/T/#m84b942a6bb838ba60ae8afd906ebbb987a577488
>
> fs/ext4/inode.c | 9 +---
> fs/ext4/namei.c | 123 +++++++++++++++++++++-------------------------
> fs/ext4/symlink.c | 44 ++++++++++++++---
> 3 files changed, 93 insertions(+), 83 deletions(-)
Hum, we don't save on code but I'd say the result is somewhat more
standard. So I guess this makes some sense. Let's see what Ted thinks...
Otherwise I've found just one small bug below.
> @@ -3270,26 +3296,8 @@ static int ext4_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> if (err)
> return err;
>
> - if ((disk_link.len > EXT4_N_BLOCKS * 4)) {
> - /*
> - * For non-fast symlinks, we just allocate inode and put it on
> - * orphan list in the first transaction => we need bitmap,
> - * group descriptor, sb, inode block, quota blocks, and
> - * possibly selinux xattr blocks.
> - */
> - credits = 4 + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb) +
> - EXT4_XATTR_TRANS_BLOCKS;
> - } else {
> - /*
> - * Fast symlink. We have to add entry to directory
> - * (EXT4_DATA_TRANS_BLOCKS + EXT4_INDEX_EXTRA_TRANS_BLOCKS),
> - * allocate new inode (bitmap, group descriptor, inode block,
> - * quota blocks, sb is already counted in previous macros).
> - */
> - credits = EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
> - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3;
> - }
> -
> + credits = EXT4_DATA_TRANS_BLOCKS(dir->i_sb) +
> + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3;
This does not seem like enough credits - we may need to allocate inode, add
entry to directory, allocate & initialize symlink block. So I think you
need to add 4 for block allocation + init in case of non-fast symlink. And
please keep the comment explaining what is actually counted in the number
of credits...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2022-04-06 18:45 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-06 8:45 [RFC PATCH] ext4: convert symlink external data block mapping to bdev Zhang Yi
2022-04-06 17:17 ` Jan Kara [this message]
2022-04-07 8:14 ` Zhang Yi
2022-04-07 13:55 ` Jan Kara
2022-04-08 5:45 ` Zhang Yi
2022-04-11 7:55 ` Jan Kara
2022-04-11 10:50 ` Zhang Yi
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=20220406171715.35euuzocoe4ljepe@quack3.lan \
--to=jack@suse.cz \
--cc=adilger.kernel@dilger.ca \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yebin10@huawei.com \
--cc=yi.zhang@huawei.com \
--cc=yukuai3@huawei.com \
/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 NNTP newsgroup(s).