On 11/11/16 11:05 AM, Omar Sandoval wrote: > On Sat, Nov 05, 2016 at 01:26:34PM -0400, je...@suse.com wrote: >> From: Jeff Mahoney <je...@suse.com> >> >> Commit 3de4586c527 (Btrfs: Allow subvolumes and snapshots anywhere >> in the directory tree) introduced the current system of placing >> snapshots in the directory tree. It also introduced the behavior of >> creating the snapshot and then creating the directory entries for it. >> >> We've kept this code around for compatibility reasons, but it turns >> out that no file systems with the old tree_root based snapshots can >> be mounted on newer (>= 2009) kernels anyway. About a month after the >> above commit, commit 2a7108ad89e (Btrfs: rev the disk format for the >> inode compat and csum selection changes) landed, changing the superblock >> magic number. >> >> As a result, we know that we'll never encounter tree_root-based dirents >> or have to deal with skipping our own snapshot dirents. Since that >> also means that we're now only iterating over DIR_INDEX items, which only >> contain one directory entry per leaf item, we don't need to loop over >> the leaf item contents anymore either. >> >> Signed-off-by: Jeff Mahoney <je...@suse.com> > > Hi, Jeff,
Hi Omar -
> One comment below.
>
>> ---
>> fs/btrfs/inode.c | 118
>> +++++++++++++++++--------------------------------------
>> 1 file changed, 37 insertions(+), 81 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 2b790bd..c52239d 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -5805,20 +5805,13 @@ static int btrfs_real_readdir(struct file *file,
>> struct dir_context *ctx)
>> int slot;
>> unsigned char d_type;
>> int over = 0;
>> - u32 di_cur;
>> - u32 di_total;
>> - u32 di_len;
>> - int key_type = BTRFS_DIR_INDEX_KEY;
>> char tmp_name[32];
>> char *name_ptr;
>> int name_len;
>> int is_curr = 0; /* ctx->pos points to the current index? */
>> bool emitted;
>> bool put = false;
>> -
>> - /* FIXME, use a real flag for deciding about the key type */
>> - if (root->fs_info->tree_root == root)
>> - key_type = BTRFS_DIR_ITEM_KEY;
>> + struct btrfs_key location;
>>
>> if (!dir_emit_dots(file, ctx))
>> return 0;
>> @@ -5829,14 +5822,11 @@ static int btrfs_real_readdir(struct file *file,
>> struct dir_context *ctx)
>>
>> path->reada = READA_FORWARD;
>>
>> - if (key_type == BTRFS_DIR_INDEX_KEY) {
>> - INIT_LIST_HEAD(&ins_list);
>> - INIT_LIST_HEAD(&del_list);
>> - put = btrfs_readdir_get_delayed_items(inode, &ins_list,
>> - &del_list);
>> - }
>> + INIT_LIST_HEAD(&ins_list);
>> + INIT_LIST_HEAD(&del_list);
>> + put = btrfs_readdir_get_delayed_items(inode, &ins_list, &del_list);
>>
>> - key.type = key_type;
>> + key.type = BTRFS_DIR_INDEX_KEY;
>> key.offset = ctx->pos;
>> key.objectid = btrfs_ino(inode);
>>
>> @@ -5862,85 +5852,53 @@ static int btrfs_real_readdir(struct file *file,
>> struct dir_context *ctx)
>>
>> if (found_key.objectid != key.objectid)
>> break;
>> - if (found_key.type != key_type)
>> + if (found_key.type != BTRFS_DIR_INDEX_KEY)
>> break;
>> if (found_key.offset < ctx->pos)
>> goto next;
>> - if (key_type == BTRFS_DIR_INDEX_KEY &&
>> - btrfs_should_delete_dir_index(&del_list,
>> - found_key.offset))
>> + if (btrfs_should_delete_dir_index(&del_list, found_key.offset))
>> goto next;
>>
>> ctx->pos = found_key.offset;
>> is_curr = 1;
>>
>> di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
>> - di_cur = 0;
>> - di_total = btrfs_item_size(leaf, item);
>> -
>> - while (di_cur < di_total) {
>> - struct btrfs_key location;
>> -
>> - if (verify_dir_item(root, leaf, di))
>> - break;
>> + if (verify_dir_item(root, leaf, di))
>> + goto next;
>>
>> - name_len = btrfs_dir_name_len(leaf, di);
>> - if (name_len <= sizeof(tmp_name)) {
>> - name_ptr = tmp_name;
>> - } else {
>> - name_ptr = kmalloc(name_len, GFP_KERNEL);
>> - if (!name_ptr) {
>> - ret = -ENOMEM;
>> - goto err;
>> - }
>> + name_len = btrfs_dir_name_len(leaf, di);
>> + if (name_len <= sizeof(tmp_name)) {
>> + name_ptr = tmp_name;
>> + } else {
>> + name_ptr = kmalloc(name_len, GFP_KERNEL);
>> + if (!name_ptr) {
>> + ret = -ENOMEM;
>> + goto err;
>> }
>> - read_extent_buffer(leaf, name_ptr,
>> - (unsigned long)(di + 1), name_len);
>> -
>> - d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
>> - btrfs_dir_item_key_to_cpu(leaf, di, &location);
>> + }
>> + read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
>> + name_len);
>>
>> + d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
>> + btrfs_dir_item_key_to_cpu(leaf, di, &location);
>>
>> - /* is this a reference to our own snapshot? If so
>> - * skip it.
>> - *
>> - * In contrast to old kernels, we insert the snapshot's
>> - * dir item and dir index after it has been created, so
>> - * we won't find a reference to our own snapshot. We
>> - * still keep the following code for backward
>> - * compatibility.
>> - */
>> - if (location.type == BTRFS_ROOT_ITEM_KEY &&
>> - location.objectid == root->root_key.objectid) {
>> - over = 0;
>> - goto skip;
>> - }
>> - over = !dir_emit(ctx, name_ptr, name_len,
>> - location.objectid, d_type);
>> + over = !dir_emit(ctx, name_ptr, name_len, location.objectid,
>> + d_type);
>>
>> -skip:
>> - if (name_ptr != tmp_name)
>> - kfree(name_ptr);
>> + if (name_ptr != tmp_name)
>> + kfree(name_ptr);
>>
>> - if (over)
>> - goto nopos;
>> - emitted = true;
>
> Shouldn't this line (getting rid of emitted) be in the next patch?
Yep. Good catch.
Thanks,
-Jeff
>> - di_len = btrfs_dir_name_len(leaf, di) +
>> - btrfs_dir_data_len(leaf, di) + sizeof(*di);
>> - di_cur += di_len;
>> - di = (struct btrfs_dir_item *)((char *)di + di_len);
>> - }
>> + if (over)
>> + goto nopos;
>> next:
>> path->slots[0]++;
>> }
>>
>> - if (key_type == BTRFS_DIR_INDEX_KEY) {
>> - if (is_curr)
>> - ctx->pos++;
>> - ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list, &emitted);
>> - if (ret)
>> - goto nopos;
>> - }
>> + if (is_curr)
>> + ctx->pos++;
>> + ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list, &emitted);
>> + if (ret)
>> + goto nopos;
>>
>> /*
>> * If we haven't emitted any dir entry, we must not touch ctx->pos as
>> @@ -5971,12 +5929,10 @@ static int btrfs_real_readdir(struct file *file,
>> struct dir_context *ctx)
>> * last entry requires it because doing so has broken 32bit apps
>> * in the past.
>> */
>> - if (key_type == BTRFS_DIR_INDEX_KEY) {
>> - if (ctx->pos >= INT_MAX)
>> - ctx->pos = LLONG_MAX;
>> - else
>> - ctx->pos = INT_MAX;
>> - }
>> + if (ctx->pos >= INT_MAX)
>> + ctx->pos = LLONG_MAX;
>> + else
>> + ctx->pos = INT_MAX;
>> nopos:
>> ret = 0;
>> err:
>> --
>> 2.7.1
>>
>> --
>> 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
>
--
Jeff Mahoney
SUSE Labs
signature.asc
Description: OpenPGP digital signature
