* Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c
[not found] <1387515880-10185-1-git-send-email-yangyongqiang01@baidu.com>
@ 2013-12-20 5:11 ` Yongqiang Yang
2013-12-20 5:11 ` Yongqiang Yang
2014-01-06 14:52 ` Theodore Ts'o
[not found] ` <1387515880-10185-2-git-send-email-yangyongqiang01@baidu.com>
1 sibling, 2 replies; 10+ messages in thread
From: Yongqiang Yang @ 2013-12-20 5:11 UTC (permalink / raw
To: Theodore Ts'o, Ext4 Developers List
From: Yongqiang Yang <xiaoqiangnk@gmail.com>
Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
---
fs/ext4/extents.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 54d52af..3654dac 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3492,7 +3492,7 @@ static int
ext4_ext_convert_to_initialized(handle_t *handle,
WARN_ON(map->m_lblk < ee_block);
/*
* It is safe to convert extent to initialized via explicit
- * zeroout only if extent is fully insde i_size or new_size.
+ * zeroout only if extent is fully inside i_size or new_size.
*/
split_flag |= ee_block + ee_len <= eof_block ? EXT4_EXT_MAY_ZEROOUT : 0;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c
2013-12-20 5:11 ` Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c Yongqiang Yang
@ 2013-12-20 5:11 ` Yongqiang Yang
2013-12-23 12:17 ` Carlos Maiolino
2014-01-06 14:52 ` Theodore Ts'o
1 sibling, 1 reply; 10+ messages in thread
From: Yongqiang Yang @ 2013-12-20 5:11 UTC (permalink / raw
To: Theodore Ts'o, Ext4 Developers List
From: Yongqiang Yang <xiaoqiangnk@gmail.com>
Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
---
fs/ext4/extents.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 54d52af..3654dac 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3492,7 +3492,7 @@ static int
ext4_ext_convert_to_initialized(handle_t *handle,
WARN_ON(map->m_lblk < ee_block);
/*
* It is safe to convert extent to initialized via explicit
- * zeroout only if extent is fully insde i_size or new_size.
+ * zeroout only if extent is fully inside i_size or new_size.
*/
split_flag |= ee_block + ee_len <= eof_block ? EXT4_EXT_MAY_ZEROOUT : 0;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
[not found] ` <1387515880-10185-2-git-send-email-yangyongqiang01@baidu.com>
@ 2013-12-20 5:13 ` Yongqiang Yang
2013-12-23 12:17 ` Carlos Maiolino
2014-01-06 14:51 ` Fwd: " Theodore Ts'o
0 siblings, 2 replies; 10+ messages in thread
From: Yongqiang Yang @ 2013-12-20 5:13 UTC (permalink / raw
To: Theodore Ts'o, Ext4 Developers List
From: Yongqiang Yang <xiaoqiangnk@gmail.com>
can be reproduced by xfstests 62 with bigalloc and 128bit size inode.
Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
---
fs/ext4/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9115f28..1869fcf 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -145,7 +145,7 @@ static int ext4_meta_trans_blocks(struct inode
*inode, int lblocks,
static int ext4_inode_is_fast_symlink(struct inode *inode)
{
int ea_blocks = EXT4_I(inode)->i_file_acl ?
- (inode->i_sb->s_blocksize >> 9) : 0;
+ EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
}
--
1.8.5.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
2013-12-20 5:13 ` Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size Yongqiang Yang
@ 2013-12-23 12:17 ` Carlos Maiolino
2013-12-23 19:33 ` Andreas Dilger
2014-01-06 14:51 ` Fwd: " Theodore Ts'o
1 sibling, 1 reply; 10+ messages in thread
From: Carlos Maiolino @ 2013-12-23 12:17 UTC (permalink / raw
To: Ext4 Developers List
On Fri, Dec 20, 2013 at 01:13:33PM +0800, Yongqiang Yang wrote:
> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>
> can be reproduced by xfstests 62 with bigalloc and 128bit size inode.
>
> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
> ---
> fs/ext4/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9115f28..1869fcf 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -145,7 +145,7 @@ static int ext4_meta_trans_blocks(struct inode
> *inode, int lblocks,
> static int ext4_inode_is_fast_symlink(struct inode *inode)
> {
> int ea_blocks = EXT4_I(inode)->i_file_acl ?
> - (inode->i_sb->s_blocksize >> 9) : 0;
> + EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
Code looks good, but looks like it has an extra TAB here. Just a cosmetic thing;
despite that, consider it
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
--
Carlos
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c
2013-12-20 5:11 ` Yongqiang Yang
@ 2013-12-23 12:17 ` Carlos Maiolino
0 siblings, 0 replies; 10+ messages in thread
From: Carlos Maiolino @ 2013-12-23 12:17 UTC (permalink / raw
To: Ext4 Developers List
On Fri, Dec 20, 2013 at 01:11:38PM +0800, Yongqiang Yang wrote:
> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>
> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
> ---
> fs/ext4/extents.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 54d52af..3654dac 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3492,7 +3492,7 @@ static int
> ext4_ext_convert_to_initialized(handle_t *handle,
> WARN_ON(map->m_lblk < ee_block);
> /*
> * It is safe to convert extent to initialized via explicit
> - * zeroout only if extent is fully insde i_size or new_size.
> + * zeroout only if extent is fully inside i_size or new_size.
> */
> split_flag |= ee_block + ee_len <= eof_block ? EXT4_EXT_MAY_ZEROOUT : 0;
>
Looks good
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
--
Carlos
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
2013-12-23 12:17 ` Carlos Maiolino
@ 2013-12-23 19:33 ` Andreas Dilger
0 siblings, 0 replies; 10+ messages in thread
From: Andreas Dilger @ 2013-12-23 19:33 UTC (permalink / raw
To: Carlos Maiolino; +Cc: Ext4 Developers List
If actually prefer if we changed this function from checking the blocks count to checking the symlink length (i_size) to determine if this is a fast or slow symlink.
I don't think there has ever been a kernel that stores symlinks < 60 chars in an external block, and it would definitely avoid the many, many times this function has been wrong because of xattrs and other things that change the number of blocks allocated to an inode. Using the block count instead of i_size makes it impossible to change the number of blocks associated with a symlink without breaking older kernels (and I'm sure this will break again in the future, just as it did when xattrs started appearing on symlinks in the first place. I'd really prefer that we move away from that.
Also, it doesn't seem obvious to me that a symlink in a bigalloc filesystem should account for ALL of the blocks in the cluster to the inode vs. just the blocks actually needed to store the symlink name.
Cheers, Andreas
> On Dec 23, 2013, at 5:17, Carlos Maiolino <cmaiolino@redhat.com> wrote:
>
>> On Fri, Dec 20, 2013 at 01:13:33PM +0800, Yongqiang Yang wrote:
>> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>>
>> can be reproduced by xfstests 62 with bigalloc and 128bit size inode.
>>
>> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
>> ---
>> fs/ext4/inode.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 9115f28..1869fcf 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -145,7 +145,7 @@ static int ext4_meta_trans_blocks(struct inode
>> *inode, int lblocks,
>> static int ext4_inode_is_fast_symlink(struct inode *inode)
>> {
>> int ea_blocks = EXT4_I(inode)->i_file_acl ?
>> - (inode->i_sb->s_blocksize >> 9) : 0;
>> + EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;
>
> Code looks good, but looks like it has an extra TAB here. Just a cosmetic thing;
> despite that, consider it
>
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
>
> --
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
2013-12-20 5:13 ` Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size Yongqiang Yang
2013-12-23 12:17 ` Carlos Maiolino
@ 2014-01-06 14:51 ` Theodore Ts'o
2014-01-06 17:47 ` Andreas Dilger
1 sibling, 1 reply; 10+ messages in thread
From: Theodore Ts'o @ 2014-01-06 14:51 UTC (permalink / raw
To: Yongqiang Yang; +Cc: Ext4 Developers List
On Fri, Dec 20, 2013 at 01:13:33PM +0800, Yongqiang Yang wrote:
> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>
> can be reproduced by xfstests 62 with bigalloc and 128bit size inode.
>
> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
Thanks, applied.
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c
2013-12-20 5:11 ` Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c Yongqiang Yang
2013-12-20 5:11 ` Yongqiang Yang
@ 2014-01-06 14:52 ` Theodore Ts'o
1 sibling, 0 replies; 10+ messages in thread
From: Theodore Ts'o @ 2014-01-06 14:52 UTC (permalink / raw
To: Yongqiang Yang; +Cc: Ext4 Developers List
On Fri, Dec 20, 2013 at 01:11:03PM +0800, Yongqiang Yang wrote:
> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>
> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
Thanks, applied.
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
2014-01-06 14:51 ` Fwd: " Theodore Ts'o
@ 2014-01-06 17:47 ` Andreas Dilger
2014-01-06 19:45 ` Theodore Ts'o
0 siblings, 1 reply; 10+ messages in thread
From: Andreas Dilger @ 2014-01-06 17:47 UTC (permalink / raw
To: Theodore Ts'o; +Cc: Yongqiang Yang, Ext4 Developers List
Ted,
What about the idea to stop using the blocks count for doing the fast/slow symlink check, and instead use i_size? IMHO this is far more robust than using the blocks count, since we've repeatedly seen bugs when the number of blocks allocated to an inode changes for done reason (e.g. xattrs, bigalloc, multi-block xattrs in the future).
AFAIK the kernel and e2fsprogs have always been consistent with symlinks <= 60 bytes being stored in the inode.
Cheers, Andreas
> On Jan 6, 2014, at 7:51, Theodore Ts'o <tytso@mit.edu> wrote:
>
>> On Fri, Dec 20, 2013 at 01:13:33PM +0800, Yongqiang Yang wrote:
>> From: Yongqiang Yang <xiaoqiangnk@gmail.com>
>>
>> can be reproduced by xfstests 62 with bigalloc and 128bit size inode.
>>
>> Signed-off-by: Yongqiang Yang <yangyongqiang01@baidu.com>
>
> Thanks, applied.
>
> - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size
2014-01-06 17:47 ` Andreas Dilger
@ 2014-01-06 19:45 ` Theodore Ts'o
0 siblings, 0 replies; 10+ messages in thread
From: Theodore Ts'o @ 2014-01-06 19:45 UTC (permalink / raw
To: Andreas Dilger; +Cc: Yongqiang Yang, Ext4 Developers List
On Mon, Jan 06, 2014 at 10:47:03AM -0700, Andreas Dilger wrote:
> What about the idea to stop using the blocks count for doing the
> fast/slow symlink check, and instead use i_size? IMHO this is far
> more robust than using the blocks count, since we've repeatedly seen
> bugs when the number of blocks allocated to an inode changes for
> done reason (e.g. xattrs, bigalloc, multi-block xattrs in the
> future).
I did see your earlier proposal on this front, but I didn't want to
this change without thinking about it a bit more closely. In
particular, we probably would want to enforce this change in e2fsck
for a while first.
Currently, if we have a slow symlink where i_size is less than 60
bytes, both e2fsprogs and the kernel handles this case. See the
attached file system image.
Yes, I created it synthetically, but keep in mind that that there are
other implementations of ext2/3/4 other than just in the Linux kernel.
In particular, the GNU Hurd and *BSD have their own independent
implementation of ext2. So even if the Linux kernel has never created
a slow symlink with i_size <= 60 bytes, but that doesn't mean that
it's for certain that there are no such implementations out there in the wild.
That doesn't mean that we should never make such a change, but it does
mean that it's not something I want to do lightly.
- Ted
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-01-06 19:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1387515880-10185-1-git-send-email-yangyongqiang01@baidu.com>
2013-12-20 5:11 ` Fwd: [RESEND PATCH 1/2] ext4: fix a typo in extents.c Yongqiang Yang
2013-12-20 5:11 ` Yongqiang Yang
2013-12-23 12:17 ` Carlos Maiolino
2014-01-06 14:52 ` Theodore Ts'o
[not found] ` <1387515880-10185-2-git-send-email-yangyongqiang01@baidu.com>
2013-12-20 5:13 ` Fwd: [RESEND PATCH 2/2] ext4: ext4_inode_is_fast_symlink should use cluster size Yongqiang Yang
2013-12-23 12:17 ` Carlos Maiolino
2013-12-23 19:33 ` Andreas Dilger
2014-01-06 14:51 ` Fwd: " Theodore Ts'o
2014-01-06 17:47 ` Andreas Dilger
2014-01-06 19:45 ` Theodore Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.