On Wed, Nov 26, 2014 at 4:19 PM, Josef Bacik <jbacik@xxxxxx> wrote:
> On 11/26/2014 11:15 AM, Filipe David Manana wrote:
>>
>> On Wed, Nov 26, 2014 at 4:07 PM, Josef Bacik <jbacik@xxxxxx> wrote:
>>>
>>> On 11/26/2014 10:28 AM, Filipe Manana wrote:
>>>>
>>>>
>>>> If the transaction handle doesn't have used blocks but has created new
>>>> block
>>>> groups make sure we turn the fs into readonly mode too. This is because
>>>> the
>>>> new block groups didn't get all their metadata persisted into the chunk
>>>> and
>>>> device trees, and therefore if a subsequent transaction starts,
>>>> allocates
>>>> space from the new block groups, writes data or metadata into that
>>>> space,
>>>> commits successfully and then after we unmount and mount the filesystem
>>>> again, the same space can be allocated again for a new block group,
>>>> resulting in file data or metadata corruption.
>>>>
>>>> Example where we don't abort the transaction when we fail to finish the
>>>> chunk allocation (add items to the chunk and device trees) and later a
>>>> future transaction where the block group is removed fails because it
>>>> can't
>>>> find the chunk item in the chunk tree:
>>>>
>>>> [25230.404300] WARNING: CPU: 0 PID: 7721 at fs/btrfs/super.c:260
>>>> __btrfs_abort_transaction+0x50/0xfc [btrfs]()
>>>> [25230.404301] BTRFS: Transaction aborted (error -28)
>>>> [25230.404302] Modules linked in: btrfs dm_flakey nls_utf8 fuse xor
>>>> raid6_pq ntfs vfat msdos fat xfs crc32c_generic libcrc32c ext3 jbd ext2
>>>> dm_mod nfsd auth_rpcgss oid_registry nfs_acl nfs lockd fscache sunrpc
>>>> loop
>>>> psmouse i2c_piix4 i2ccore parport_pc parport processor button pcspkr
>>>> serio_raw thermal_sys evdev microcode ext4 crc16 jbd2 mbcache sr_mod
>>>> cdrom
>>>> ata_generic sg sd_mod crc_t10dif crct10dif_generic crct10dif_common
>>>> virtio_scsi floppy e1000 ata_piix libata virtio_pci virtio_ring scsi_mod
>>>> virtio [last unloaded: btrfs]
>>>> [25230.404325] CPU: 0 PID: 7721 Comm: xfs_io Not tainted
>>>> 3.17.0-rc5-btrfs-next-1+ #1
>>>> [25230.404326] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>>>> BIOS
>>>> rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
>>>> [25230.404328] 0000000000000000 ffff88004581bb08 ffffffff813e7a13
>>>> ffff88004581bb50
>>>> [25230.404330] ffff88004581bb40 ffffffff810423aa ffffffffa049386a
>>>> 00000000ffffffe4
>>>> [25230.404332] ffffffffa05214c0 000000000000240c ffff88010fc8f800
>>>> ffff88004581bba8
>>>> [25230.404334] Call Trace:
>>>> [25230.404338] [<ffffffff813e7a13>] dump_stack+0x45/0x56
>>>> [25230.404342] [<ffffffff810423aa>] warn_slowpath_common+0x7f/0x98
>>>> [25230.404351] [<ffffffffa049386a>] ?
>>>> __btrfs_abort_transaction+0x50/0xfc
>>>> [btrfs]
>>>> [25230.404353] [<ffffffff8104240b>] warn_slowpath_fmt+0x48/0x50
>>>> [25230.404362] [<ffffffffa049386a>] __btrfs_abort_transaction+0x50/0xfc
>>>> [btrfs]
>>>> [25230.404374] [<ffffffffa04a8c43>]
>>>> btrfs_create_pending_block_groups+0x10c/0x135 [btrfs]
>>>> [25230.404387] [<ffffffffa04b77fd>] __btrfs_end_transaction+0x7e/0x2de
>>>> [btrfs]
>>>> [25230.404398] [<ffffffffa04b7a6d>] btrfs_end_transaction+0x10/0x12
>>>> [btrfs]
>>>> [25230.404408] [<ffffffffa04a3d64>]
>>>> btrfs_check_data_free_space+0x111/0x1f0 [btrfs]
>>>> [25230.404421] [<ffffffffa04c53bd>] __btrfs_buffered_write+0x160/0x48d
>>>> [btrfs]
>>>> [25230.404425] [<ffffffff811a9268>] ? cap_inode_need_killpriv+0x2d/0x37
>>>> [25230.404429] [<ffffffff810f6501>] ? get_page+0x1a/0x2b
>>>> [25230.404441] [<ffffffffa04c7c95>] btrfs_file_write_iter+0x321/0x42f
>>>> [btrfs]
>>>> [25230.404443] [<ffffffff8110f5d9>] ? handle_mm_fault+0x7f3/0x846
>>>> [25230.404446] [<ffffffff813e98c5>] ? mutex_unlock+0x16/0x18
>>>> [25230.404449] [<ffffffff81138d68>] new_sync_write+0x7c/0xa0
>>>> [25230.404450] [<ffffffff81139401>] vfs_write+0xb0/0x112
>>>> [25230.404452] [<ffffffff81139c9d>] SyS_pwrite64+0x66/0x84
>>>> [25230.404454] [<ffffffff813ebf52>] system_call_fastpath+0x16/0x1b
>>>> [25230.404455] ---[ end trace 5aa5684fdf47ab38 ]---
>>>> [25230.404458] BTRFS warning (device sdc):
>>>> btrfs_create_pending_block_groups:9228: Aborting unused transaction(No
>>>> space
>>>> left).
>>>> [25288.084814] BTRFS: error (device sdc) in btrfs_free_chunk:2509:
>>>> errno=-2 No such entry (Failed lookup while freeing chunk.)
>>>>
>>>> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
>>>> ---
>>>> fs/btrfs/extent-tree.c | 5 +++--
>>>> fs/btrfs/super.c | 2 +-
>>>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>>> index 4bf8f02..0a5e770 100644
>>>> --- a/fs/btrfs/extent-tree.c
>>>> +++ b/fs/btrfs/extent-tree.c
>>>> @@ -9209,9 +9209,8 @@ void btrfs_create_pending_block_groups(struct
>>>> btrfs_trans_handle *trans,
>>>> int ret = 0;
>>>>
>>>> list_for_each_entry_safe(block_group, tmp, &trans->new_bgs,
>>>> bg_list) {
>>>> - list_del_init(&block_group->bg_list);
>>>> if (ret)
>>>> - continue;
>>>> + goto next;
>>>>
>>>> spin_lock(&block_group->lock);
>>>> memcpy(&item, &block_group->item, sizeof(item));
>>>> @@ -9226,6 +9225,8 @@ void btrfs_create_pending_block_groups(struct
>>>> btrfs_trans_handle *trans,
>>>> key.objectid,
>>>> key.offset);
>>>> if (ret)
>>>> btrfs_abort_transaction(trans, extent_root,
>>>> ret);
>>>> +next:
>>>> + list_del_init(&block_group->bg_list);
>>>> }
>>>> }
>>>>
>>>
>>> I don't understand this change, logically it seems the same as what we
>>> had
>>> before. Thanks,
>>
>>
>> It isn't. Before we would not turn the fs readonly if the transaction
>> had no blocks used but had new block groups. This change just makes it
>> turn into readonly mode if it has new block groups (even if no blocks
>> were used).
>> See the trace in the log where it shows we failed to finish the chunk
>> allocation but the transaction was aborted and didn't turn the fs into
>> readonly mode.
>>
>
> Yeah the bit below makes sense, its the above change that is the same.
> Before we deleted the entry from the list and then if there is an error we
> just continue, now if there is an error you go to next and delete the entry
> and continue, which is the same thing. Thanks,
I don't see how it's the same.
Before if we had a single new block group, when we called
abort_transaction the list of block groups was empty because we
removed the bg from the list before calling abort_transaction. This
change just ensures that for the single new block group case
abort_transaction sees there's a new block group and takes the action
of making the fs readonly.
>
> Josef
>
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
