In many parts of code map_extent_buffer() return value is not handled.
What is this function doing? Essentially calling kmap_atomic()
using the KM_USER1 memory, right? This memory is only one page in
size, and so it may failed easily?
map_extent_buffer(parent,
btrfs_node_key_ptr_offset(i),
sizeof(struct btrfs_key_ptr),
&parent->map_token,
&parent->kaddr,
&parent->map_start,
&parent->map_len,
KM_USER1);
Personally, I don't think this API is wise to use kmap_atomic(), as in
many parts of kernel codes, in between kmap_ and kunmap_ it is just a
few instructions from each other. But here I can see like this:
ctree.c:
if (!parent->map_token) {
map_extent_buffer(parent,
btrfs_node_key_ptr_offset(i),
sizeof(struct btrfs_key_ptr),
&parent->map_token,
&parent->kaddr,
&parent->map_start,
&parent->map_len,
KM_USER1);
}
btrfs_node_key(parent, &disk_key, i);
if (!progress_passed && comp_keys(&disk_key,
progress) < 0)
continue;
progress_passed = 1;
blocknr = btrfs_node_blockptr(parent, i);
if (last_block == 0)
last_block = blocknr;
if (i > 0) {
other = btrfs_node_blockptr(parent, i - 1);
close = close_blocks(blocknr, other, blocksize);
}
if (close && i < end_slot - 2) {
other = btrfs_node_blockptr(parent, i + 1);
close = close_blocks(blocknr, other, blocksize);
}
if (close) {
last_block = blocknr;
continue;
}
if (parent->map_token) {
unmap_extent_buffer(parent, parent->map_token,
KM_USER1);
parent->map_token = NULL;
}
cur = btrfs_find_tree_block(root, blocknr, blocksize);
So problems like leakage can easily occurred. For example, after the
map_extent_buffer(), followed by the "continue", the
unmap_extent_buffer() will not be called, a problem???? I am not sure,
as I don't fully understand the code yet :-). (And the
map_extent_buffer will be called again, using KM_USER1, without the
previous one being unmap?).
--
Regards,
Peter Teoh
> So problems like leakage can easily occurred. For example, after the > map_extent_buffer(), followed by the "continue", the > unmap_extent_buffer() will not be called, a problem???? I am not sure, > as I don't fully understand the code yet :-). (And the > map_extent_buffer will be called again, using KM_USER1, without the > previous one being unmap?).First, at the end of all of these (that is to say, the end of function btrfs_realloc_node), there is: if (parent->map_token) { unmap_extent_buffer(parent, parent->map_token,KM_USER1); parent->map_token = NULL; } return err; It can make sure that we can call unmap_extent_buffer anyway when we get out of the function (of course, if parent->map_token != NULL). Also, at the very beginning of map_extent_buffer, there is : if (eb->map_token) { unmap_extent_buffer(eb, eb->map_token, km); eb->map_token = NULL; save = 1; } So if we feed a mapped extent buffer into the function, it will unmap it at first, and then call map_private_extent_buffer to map it. Regards, ZYH -- - Zhu Yanhai
On Sunday 16 March 2008, Peter Teoh wrote:> In many parts of code map_extent_buffer() return value is not handled. > What is this function doing? Essentially calling kmap_atomic() > using the KM_USER1 memory, right? This memory is only one page in > size, and so it may failed easily? > > map_extent_buffer(parent, > btrfs_node_key_ptr_offset(i), > sizeof(struct btrfs_key_ptr), > &parent->map_token, &parent->kaddr, > &parent->map_start, > &parent->map_len, KM_USER1); > > Personally, I don't think this API is wise to use kmap_atomic(), as in > many parts of kernel codes, in between kmap_ and kunmap_ it is just a > few instructions from each other. But here I can see like this: >kmap_atomic is used because the page may be a highmem page. We are required to do extra steps to access the contents of the page, but it allows us to use any page on the system as opposed to just the first 1GB of lowmem pages (on i386 at least). x86-64 doesn't have such restrictions and kmap atomic is inexpensive there (but not entirely free). map_extent_buffer has a number of different uses. It caches mappings so that repeated calls can be done by all of the btrfs_set/get functions in tight loops. If it is called with an existing mapping, the old mapping is unmapped before it continues. It can return an error, but only when it is called incorrectly. Older versions had BUG_ONs and WARN_ONs in various places to find those errors. (Yes, there are a number of places in btrfs right now that need proper error checking. The code churn is very high right now, and I'm focusing on other bits at the moment). -chris