At 06/01/2016 12:15 AM, David Sterba wrote:
On Tue, May 31, 2016 at 11:08:39AM +0800, luke wrote:
+};
+
+/* dynamically allocate and initialize a ref_root */
+static struct ref_root *ref_root_alloc(gfp_t gfp_mask)
+{
+       struct ref_root *ref_tree;
+
+       ref_tree = kmalloc(sizeof(*ref_tree), gfp_mask);
Drop the gfp_mask and make it GFP_KERNEL
OK, I'll drop the gfp_mask, but can you tell me why should use
GFP_KERNEL instead of GFP_NOFS?
Because there's no need to narrow the allocation constraints. GFP_NOFS
is necessary when the caller is on a critical path that must not recurse
back to the filesystem through the allocation (ie. if the allocator
decides to free some memory and tries tro write dirty data). FIEMAP is
called from an ioctl.
OK, I'll update this patch with GFP_KERNEL.

                        disko = em->block_start + offset_in_extent;
/*
+                        * We need a trans handle to get delayed refs
+                        */
+                       trans = btrfs_join_transaction(root);
What are the implications of join/end transaction here? It's just
fiemap, I would not expect messing with transaction here.
This transaction is used to handle delayed_refs, if trans = NULL, we
have to ignore the delayed_refs.
Yes that's what the comment says as well, but I still don't understand
why, so I need somebody else to review that part.
If not checking delayed refs, shared_flag will not be set for reflink until transaction committed.
Test case to test this misbehavior is already submitted(generic/353).

The simplest test procedure would be like  the following:

# xfs_io -f -c "pwrite 0 128K" test
# cp --reflink test test2
# xfs_io -c "fiemap" test2

And normally, the SHARED flag should be set, but since the trans isn't committed yet, if not checking delayed refs, we can't determine whether it is shared by others.

Thanks,
Lu






--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to