From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ig0-f174.google.com ([209.85.213.174]:34393 "EHLO mail-ig0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751196AbbJ0JWu (ORCPT ); Tue, 27 Oct 2015 05:22:50 -0400 Received: by igpw7 with SMTP id w7so1733480igp.1 for ; Tue, 27 Oct 2015 02:22:47 -0700 (PDT) MIME-Version: 1.0 Reply-To: fdmanana@gmail.com In-Reply-To: <562EF9D7.50101@cn.fujitsu.com> References: <1444702827-18169-1-git-send-email-quwenruo@cn.fujitsu.com> <1444702827-18169-7-git-send-email-quwenruo@cn.fujitsu.com> <562EF9D7.50101@cn.fujitsu.com> Date: Tue, 27 Oct 2015 09:22:47 +0000 Message-ID: Subject: Re: [PATCH v3 06/21] btrfs: delayed_ref: Add new function to record reserved space into delayed ref From: Filipe Manana To: Qu Wenruo Cc: "linux-btrfs@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Tue, Oct 27, 2015 at 4:13 AM, Qu Wenruo wrote: > > > Filipe Manana wrote on 2015/10/25 14:39 +0000: >> >> On Tue, Oct 13, 2015 at 3:20 AM, Qu Wenruo >> wrote: >>> >>> Add new function btrfs_add_delayed_qgroup_reserve() function to record >>> how much space is reserved for that extent. >>> >>> As btrfs only accounts qgroup at run_delayed_refs() time, so newly >>> allocated extent should keep the reserved space until then. >>> >>> So add needed function with related members to do it. >>> >>> Signed-off-by: Qu Wenruo >>> --- >>> v2: >>> None >>> v3: >>> None >>> --- >>> fs/btrfs/delayed-ref.c | 29 +++++++++++++++++++++++++++++ >>> fs/btrfs/delayed-ref.h | 14 ++++++++++++++ >>> 2 files changed, 43 insertions(+) >>> >>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c >>> index ac3e81d..bd9b63b 100644 >>> --- a/fs/btrfs/delayed-ref.c >>> +++ b/fs/btrfs/delayed-ref.c >>> @@ -476,6 +476,8 @@ add_delayed_ref_head(struct btrfs_fs_info *fs_info, >>> INIT_LIST_HEAD(&head_ref->ref_list); >>> head_ref->processing = 0; >>> head_ref->total_ref_mod = count_mod; >>> + head_ref->qgroup_reserved = 0; >>> + head_ref->qgroup_ref_root = 0; >>> >>> /* Record qgroup extent info if provided */ >>> if (qrecord) { >>> @@ -746,6 +748,33 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info >>> *fs_info, >>> return 0; >>> } >>> >>> +int btrfs_add_delayed_qgroup_reserve(struct btrfs_fs_info *fs_info, >>> + struct btrfs_trans_handle *trans, >>> + u64 ref_root, u64 bytenr, u64 >>> num_bytes) >>> +{ >>> + struct btrfs_delayed_ref_root *delayed_refs; >>> + struct btrfs_delayed_ref_head *ref_head; >>> + int ret = 0; >>> + >>> + if (!fs_info->quota_enabled || !is_fstree(ref_root)) >>> + return 0; >>> + >>> + delayed_refs = &trans->transaction->delayed_refs; >>> + >>> + spin_lock(&delayed_refs->lock); >>> + ref_head = find_ref_head(&delayed_refs->href_root, bytenr, 0); >>> + if (!ref_head) { >>> + ret = -ENOENT; >>> + goto out; >>> + } >> >> >> Hi Qu, >> >> So while running btrfs/063, with qgroups enabled (I modified the test >> to enable qgroups), ran into this 2 times: >> >> [169125.246506] BTRFS info (device sdc): disk space caching is enabled >> [169125.363164] ------------[ cut here ]------------ >> [169125.365236] WARNING: CPU: 10 PID: 2827 at fs/btrfs/inode.c:2929 >> btrfs_finish_ordered_io+0x347/0x4eb [btrfs]() >> [169125.367702] BTRFS: Transaction aborted (error -2) >> [169125.368830] Modules linked in: btrfs dm_flakey dm_mod >> crc32c_generic xor raid6_pq nfsd auth_rpcgss oid_registry nfs_acl nfs >> lockd grace fscache sunrpc loop fuse parport_pc parport i2c_piix4 >> psmouse acpi_cpufreq microcode pcspkr processor evdev i2c_core >> serio_raw button ext4 crc16 jbd2 mbcache sd_mod sg sr_mod cdrom >> ata_generic virtio_scsi ata_piix libata floppy virtio_pci virtio_ring >> scsi_mod e1000 virtio [last unloaded: btrfs] >> [169125.376755] CPU: 10 PID: 2827 Comm: kworker/u32:14 Tainted: G >> W 4.3.0-rc5-btrfs-next-17+ #1 > > > Hi Filipe, Hi Qu, > > Although not related to the bug report, I'm a little interested in your > testing kernel. > > Are you testing integration-4.4 from Chris repo? Yes, I got that from Chris' integration-4.4 branch. > Or 4.3-rc from mainline repo with my qgroup reserve patchset applied? > > Although integration-4.4 already merged qgroup reserve patchset, but it's > causing some strange bug like over decrease data sinfo->bytes_may_use, > mainly in generic/127 testcase. Haven't hit that one yet. > > But if qgroup reserve patchset is rebased to integration-4.3 (I did all my > old tests based on that), no generic/127 problem at all. > > Thanks, > Qu > > >> [169125.378522] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), >> BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org >> 04/01/2014 >> [169125.380916] Workqueue: btrfs-endio-write btrfs_endio_write_helper >> [btrfs] >> [169125.382167] 0000000000000000 ffff88007ef2bc28 ffffffff812566f4 >> ffff88007ef2bc70 >> [169125.383643] ffff88007ef2bc60 ffffffff8104d0a6 ffffffffa03cac33 >> ffff8801f5ca6db0 >> [169125.385197] ffff8802c6c7ee98 ffff880122bc1000 00000000fffffffe >> ffff88007ef2bcc8 >> [169125.386691] Call Trace: >> [169125.387194] [] dump_stack+0x4e/0x79 >> [169125.388205] [] warn_slowpath_common+0x9f/0xb8 >> [169125.389386] [] ? >> btrfs_finish_ordered_io+0x347/0x4eb [btrfs] >> [169125.390837] [] warn_slowpath_fmt+0x48/0x50 >> [169125.391839] [] ? unpin_extent_cache+0xbe/0xcc >> [btrfs] >> [169125.392973] [] >> btrfs_finish_ordered_io+0x347/0x4eb [btrfs] >> [169125.395714] [] ? >> _raw_spin_unlock_irqrestore+0x38/0x60 >> [169125.396888] [] ? >> trace_hardirqs_off_caller+0x1f/0xb9 >> [169125.397986] [] finish_ordered_fn+0x15/0x17 [btrfs] >> [169125.399122] [] normal_work_helper+0x14c/0x32a >> [btrfs] >> [169125.400300] [] btrfs_endio_write_helper+0x12/0x14 >> [btrfs] >> [169125.401450] [] process_one_work+0x24a/0x4ac >> [169125.402631] [] worker_thread+0x206/0x2c2 >> [169125.403622] [] ? rescuer_thread+0x2cb/0x2cb >> [169125.404693] [] kthread+0xef/0xf7 >> [169125.405727] [] ? kthread_parkme+0x24/0x24 >> [169125.406808] [] ret_from_fork+0x3f/0x70 >> [169125.407834] [] ? kthread_parkme+0x24/0x24 >> [169125.408840] ---[ end trace 6ee4342a5722b119 ]--- >> [169125.409654] BTRFS: error (device sdc) in >> btrfs_finish_ordered_io:2929: errno=-2 No such entry >> >> So what you have here is racy: >> >> btrfs_finish_ordered_io() >> joins existing transaction (or starts a new one) >> insert_reserved_file_extent() >> btrfs_alloc_reserved_file_extent() --> creates delayed ref >> >> ******* delayed refs are run, someone called >> btrfs_async_run_delayed_refs() from btrfs_end_transaction(), ref head >> is removed ****** >> >> btrfs_add_delayed_qgroup_reserve() --> does not find delayed ref >> head, returns -ENOENT and finish_ordered_io aborts current >> transaction... >> >> A very tiny race, but... >> >> thanks >> >> >>> + WARN_ON(ref_head->qgroup_reserved || ref_head->qgroup_ref_root); >>> + ref_head->qgroup_ref_root = ref_root; >>> + ref_head->qgroup_reserved = num_bytes; >>> +out: >>> + spin_unlock(&delayed_refs->lock); >>> + return ret; >>> +} >>> + >>> int btrfs_add_delayed_extent_op(struct btrfs_fs_info *fs_info, >>> struct btrfs_trans_handle *trans, >>> u64 bytenr, u64 num_bytes, >>> diff --git a/fs/btrfs/delayed-ref.h b/fs/btrfs/delayed-ref.h >>> index 13fb5e6..d4c41e2 100644 >>> --- a/fs/btrfs/delayed-ref.h >>> +++ b/fs/btrfs/delayed-ref.h >>> @@ -113,6 +113,17 @@ struct btrfs_delayed_ref_head { >>> int total_ref_mod; >>> >>> /* >>> + * For qgroup reserved space freeing. >>> + * >>> + * ref_root and reserved will be recorded after >>> + * BTRFS_ADD_DELAYED_EXTENT is called. >>> + * And will be used to free reserved qgroup space at >>> + * run_delayed_refs() time. >>> + */ >>> + u64 qgroup_ref_root; >>> + u64 qgroup_reserved; >>> + >>> + /* >>> * when a new extent is allocated, it is just reserved in memory >>> * The actual extent isn't inserted into the extent allocation >>> tree >>> * until the delayed ref is processed. must_insert_reserved is >>> @@ -242,6 +253,9 @@ int btrfs_add_delayed_data_ref(struct btrfs_fs_info >>> *fs_info, >>> u64 owner, u64 offset, int action, >>> struct btrfs_delayed_extent_op >>> *extent_op, >>> int no_quota); >>> +int btrfs_add_delayed_qgroup_reserve(struct btrfs_fs_info *fs_info, >>> + struct btrfs_trans_handle *trans, >>> + u64 ref_root, u64 bytenr, u64 >>> num_bytes); >>> int btrfs_add_delayed_extent_op(struct btrfs_fs_info *fs_info, >>> struct btrfs_trans_handle *trans, >>> u64 bytenr, u64 num_bytes, >>> -- >>> 2.6.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> > -- 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."