Re: [PATCH 1/2] btrfs: remove old tree_root dirent processing in btrfs_real_readdir()

[David Sterba]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs: remove old tree_root dirent processing in btrfs_real_readdir()

[Jeff Mahoney]

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 
>>
>> 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 
> 
> 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(_list);
>> -INIT_LIST_HEAD(_list);
>> -put = btrfs_readdir_get_delayed_items(inode, _list,
>> -  _list);
>> -}
>> +INIT_LIST_HEAD(_list);
>> +INIT_LIST_HEAD(_list);
>> +put = btrfs_readdir_get_delayed_items(inode, _list, _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(_list,
>> -  found_key.offset))
>> +if (btrfs_should_delete_dir_index(_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 

Re: [PATCH 1/2] btrfs: remove old tree_root dirent processing in btrfs_real_readdir()

[Omar Sandoval]

On Sat, Nov 05, 2016 at 01:26:34PM -0400, je...@suse.com wrote:
> From: Jeff Mahoney 
> 
> 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 

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(_list);
> - INIT_LIST_HEAD(_list);
> - put = btrfs_readdir_get_delayed_items(inode, _list,
> -   _list);
> - }
> + INIT_LIST_HEAD(_list);
> + INIT_LIST_HEAD(_list);
> + put = btrfs_readdir_get_delayed_items(inode, _list, _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(_list,
> -   found_key.offset))
> + if (btrfs_should_delete_dir_index(_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;
>

Re: [PATCH 1/2] btrfs: remove old tree_root dirent processing in btrfs_real_readdir()

[David Sterba]

On Sat, Nov 05, 2016 at 01:26:34PM -0400, je...@suse.com wrote:
> From: Jeff Mahoney 
> 
> 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 

Reviewed-by: David Sterba 
--
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