| Message ID | 1478366795-27009-1-git-send-email-jeffm@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Nov 05, 2016 at 01:26:34PM -0400, jeffm@suse.com wrote: > From: Jeff Mahoney <jeffm@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 <jeffm@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> -- 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
On Sat, Nov 05, 2016 at 01:26:34PM -0400, jeffm@suse.com wrote: > From: Jeff Mahoney <jeffm@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 <jeffm@suse.com> Hi, Jeff, 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? > - 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/11/16 11:05 AM, Omar Sandoval wrote: > On Sat, Nov 05, 2016 at 01:26:34PM -0400, jeffm@suse.com wrote: >> From: Jeff Mahoney <jeffm@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 <jeffm@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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Nov 14, 2016 at 09:08:14PM -0500, Jeff Mahoney wrote: > >> - */ > >> - 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. Updated. -- 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
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; - 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: