diff mbox

xfs: clear di_forkoff on ialloc

Message ID bb3e00eb-7334-1ac5-509f-89804583e9ff@suse.com
State New, archived
Headers show

Commit Message

Jeff Mahoney Oct. 5, 2016, 5:04 p.m. UTC
Commit 6dfe5a049f2 (xfs: xfs_attr_inactive leaves inconsistent
attr fork state behind) fixed an issue where an inconsistent
attr fork count persisted on disk if there was concurrent inode
writeback happening after the inode was evicted from the VFS layer.

If one of those inodes landed on disk and was reused, it may have
an invalid di_forkoff, which can cause problems when trying to add
new extended attributes.  Since we clear the rest of the attribute
fork values on ialloc, let's clear di_forkoff as well and ensure the
invalid value won't be encountered.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/xfs/xfs_inode.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Eric Sandeen June 29, 2017, 10:34 p.m. UTC | #1
On 10/5/16 12:04 PM, Jeff Mahoney wrote:
> Commit 6dfe5a049f2 (xfs: xfs_attr_inactive leaves inconsistent
> attr fork state behind) fixed an issue where an inconsistent
> attr fork count persisted on disk if there was concurrent inode
> writeback happening after the inode was evicted from the VFS layer.
> 
> If one of those inodes landed on disk and was reused, it may have
> an invalid di_forkoff, which can cause problems when trying to add
> new extended attributes.  Since we clear the rest of the attribute
> fork values on ialloc, let's clear di_forkoff as well and ensure the
> invalid value won't be encountered.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

This makes sense to me, but seems to have gotten lost.

It'd need to go to libxfs_ialloc as well if it makes the kernel.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_inode.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -835,6 +835,7 @@ xfs_ialloc(
>  	 */
>  	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
>  	ip->i_d.di_anextents = 0;
> +	ip->i_d.di_forkoff = 0;
>  
>  	/*
>  	 * Log the new values stuffed into the inode.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen June 29, 2017, 10:41 p.m. UTC | #2
On 6/29/17 5:34 PM, Eric Sandeen wrote:
> On 10/5/16 12:04 PM, Jeff Mahoney wrote:
>> Commit 6dfe5a049f2 (xfs: xfs_attr_inactive leaves inconsistent
>> attr fork state behind) fixed an issue where an inconsistent
>> attr fork count persisted on disk if there was concurrent inode
>> writeback happening after the inode was evicted from the VFS layer.
>>
>> If one of those inodes landed on disk and was reused, it may have
>> an invalid di_forkoff, which can cause problems when trying to add
>> new extended attributes.  Since we clear the rest of the attribute
>> fork values on ialloc, let's clear di_forkoff as well and ensure the
>> invalid value won't be encountered.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> 
> This makes sense to me, but seems to have gotten lost.
> 
> It'd need to go to libxfs_ialloc as well if it makes the kernel.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Meh, I missed Dave's "it's fine but it shouldn't matter" reply,
sorry.  Still, seems like it's not a bad idea to initialize this
along with everything else.

>> ---
>>  fs/xfs/xfs_inode.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -835,6 +835,7 @@ xfs_ialloc(
>>  	 */
>>  	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
>>  	ip->i_d.di_anextents = 0;
>> +	ip->i_d.di_forkoff = 0;
>>  
>>  	/*
>>  	 * Log the new values stuffed into the inode.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -835,6 +835,7 @@  xfs_ialloc(
 	 */
 	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
 	ip->i_d.di_anextents = 0;
+	ip->i_d.di_forkoff = 0;
 
 	/*
 	 * Log the new values stuffed into the inode.