[PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Sachin Kamat]

ext4_extents.h was included twice.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 fs/ext4/super.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ad6cd8a..c8a6138 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -50,7 +50,6 @@
 #include "xattr.h"
 #include "acl.h"
 #include "mballoc.h"
-#include "ext4_extents.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/ext4.h>
-- 
1.7.4.1

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Zheng Liu]

On Mon, Nov 19, 2012 at 04:47:22PM +0530, Sachin Kamat wrote:
> ext4_extents.h was included twice.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  fs/ext4/super.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ad6cd8a..c8a6138 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -50,7 +50,6 @@
>  #include "xattr.h"
>  #include "acl.h"
>  #include "mballoc.h"
> -#include "ext4_extents.h"

Hi Sachin,

Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.

Regards,
                                                - Zheng

>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/ext4.h>
> -- 
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Theodore Ts'o]

On Mon, Nov 19, 2012 at 09:39:45PM +0800, Zheng Liu wrote:
> Hi Sachin,
> 
> Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.

It's there because ext4.h includes ext4_extents.h -- at the end of the
header file, where it's not quite as obvious.

What we should probably do is move the function declarations into
ext4.h, and then see if we can isolate the number of fs/ext4/*.c files
that are aware of the on-disk extents encoding, such that it doesn't
make sense to #include ext4_extenst.h from the ext4.h header file.

It's mainly a cleanup thing, but it would probably also help if we
ever want to support alternate extents encodings (for example to
support a full 64-bit physical block numbers, or more likely, more
than 32 bits worth of logical block nunbers --- so we can test large
file systems natively using ext4, instead of using xfs, which is what
I currently do).  That's a low priority thing in my book, but if
someone is interesting in taking on the project, they should let me
know.

   	     	 	       	      	  - Ted

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Sachin Kamat]

Hi Zheng,

On 19 November 2012 19:09, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> On Mon, Nov 19, 2012 at 04:47:22PM +0530, Sachin Kamat wrote:
>> ext4_extents.h was included twice.
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  fs/ext4/super.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index ad6cd8a..c8a6138 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -50,7 +50,6 @@
>>  #include "xattr.h"
>>  #include "acl.h"
>>  #include "mballoc.h"
>> -#include "ext4_extents.h"
>
> Hi Sachin,
>
> Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.

The duplicated code is available on the linux-next tree (20121115).
The second instance was added by the commit
"ext4: let ext4 maintain extent status tree" (commit id: 51865fda).

>
> Regards,
>                                                 - Zheng
>
>>
>>  #define CREATE_TRACE_POINTS
>>  #include <trace/events/ext4.h>
>> --
>> 1.7.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With warm regards,
Sachin

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Zheng Liu]

On Mon, Nov 19, 2012 at 10:00:00AM -0500, Theodore Ts'o wrote:
> On Mon, Nov 19, 2012 at 09:39:45PM +0800, Zheng Liu wrote:
> > Hi Sachin,
> > 
> > Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.
> 
> It's there because ext4.h includes ext4_extents.h -- at the end of the
> header file, where it's not quite as obvious.

Ah, I see.  Thanks for pointing out.

> 
> What we should probably do is move the function declarations into
> ext4.h, and then see if we can isolate the number of fs/ext4/*.c files
> that are aware of the on-disk extents encoding, such that it doesn't
> make sense to #include ext4_extenst.h from the ext4.h header file.
> 
> It's mainly a cleanup thing, but it would probably also help if we
> ever want to support alternate extents encodings (for example to
> support a full 64-bit physical block numbers, or more likely, more
> than 32 bits worth of logical block nunbers --- so we can test large
> file systems natively using ext4, instead of using xfs, which is what
> I currently do).  That's a low priority thing in my book, but if
> someone is interesting in taking on the project, they should let me
> know.

Cool!  Thanks for sharing this information with us.

Regards,
                                                - Zheng

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Zheng Liu]

On Mon, Nov 19, 2012 at 08:36:06PM +0530, Sachin Kamat wrote:
> Hi Zheng,
> 
> On 19 November 2012 19:09, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> > On Mon, Nov 19, 2012 at 04:47:22PM +0530, Sachin Kamat wrote:
> >> ext4_extents.h was included twice.
> >>
> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >>  fs/ext4/super.c |    1 -
> >>  1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> >> index ad6cd8a..c8a6138 100644
> >> --- a/fs/ext4/super.c
> >> +++ b/fs/ext4/super.c
> >> @@ -50,7 +50,6 @@
> >>  #include "xattr.h"
> >>  #include "acl.h"
> >>  #include "mballoc.h"
> >> -#include "ext4_extents.h"
> >
> > Hi Sachin,
> >
> > Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.
> 
> The duplicated code is available on the linux-next tree (20121115).
> The second instance was added by the commit
> "ext4: let ext4 maintain extent status tree" (commit id: 51865fda).

Good catch.  Sorry for my fault that introduces this duplicated code.
The patch looks good to me.

Regards,
                                                - Zheng

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Sachin Kamat]

Hi Theodore,

On 19 November 2012 21:53, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> On Mon, Nov 19, 2012 at 08:36:06PM +0530, Sachin Kamat wrote:
>> Hi Zheng,
>>
>> On 19 November 2012 19:09, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
>> > On Mon, Nov 19, 2012 at 04:47:22PM +0530, Sachin Kamat wrote:
>> >> ext4_extents.h was included twice.
>> >>
>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> >> ---
>> >>  fs/ext4/super.c |    1 -
>> >>  1 files changed, 0 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> >> index ad6cd8a..c8a6138 100644
>> >> --- a/fs/ext4/super.c
>> >> +++ b/fs/ext4/super.c
>> >> @@ -50,7 +50,6 @@
>> >>  #include "xattr.h"
>> >>  #include "acl.h"
>> >>  #include "mballoc.h"
>> >> -#include "ext4_extents.h"
>> >
>> > Hi Sachin,
>> >
>> > Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.
>>
>> The duplicated code is available on the linux-next tree (20121115).
>> The second instance was added by the commit
>> "ext4: let ext4 maintain extent status tree" (commit id: 51865fda).
>
> Good catch.  Sorry for my fault that introduces this duplicated code.
> The patch looks good to me.
>
> Regards,
>                                                 - Zheng

This is just a simple patch to remove the include statement coded
twice in the file.
Please pick this up.

-- 
With warm regards,
Sachin

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Sachin Kamat]

ping

On 22 November 2012 10:43, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Theodore,
>
> On 19 November 2012 21:53, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
>> On Mon, Nov 19, 2012 at 08:36:06PM +0530, Sachin Kamat wrote:
>>> Hi Zheng,
>>>
>>> On 19 November 2012 19:09, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
>>> > On Mon, Nov 19, 2012 at 04:47:22PM +0530, Sachin Kamat wrote:
>>> >> ext4_extents.h was included twice.
>>> >>
>>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>>> >> ---
>>> >>  fs/ext4/super.c |    1 -
>>> >>  1 files changed, 0 insertions(+), 1 deletions(-)
>>> >>
>>> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>>> >> index ad6cd8a..c8a6138 100644
>>> >> --- a/fs/ext4/super.c
>>> >> +++ b/fs/ext4/super.c
>>> >> @@ -50,7 +50,6 @@
>>> >>  #include "xattr.h"
>>> >>  #include "acl.h"
>>> >>  #include "mballoc.h"
>>> >> -#include "ext4_extents.h"
>>> >
>>> > Hi Sachin,
>>> >
>>> > Sorry, I don't find this duplicated code in mainline kernel 3.7-rc6.
>>>
>>> The duplicated code is available on the linux-next tree (20121115).
>>> The second instance was added by the commit
>>> "ext4: let ext4 maintain extent status tree" (commit id: 51865fda).
>>
>> Good catch.  Sorry for my fault that introduces this duplicated code.
>> The patch looks good to me.
>>
>> Regards,
>>                                                 - Zheng
>
> This is just a simple patch to remove the include statement coded
> twice in the file.
> Please pick this up.
>
> --
> With warm regards,
> Sachin



-- 
With warm regards,
Sachin

Re: [PATCH 1/1] ext4: Remove duplicate inclusion of ext4_extents.h in super.c

[Theodore Ts'o]

This is the patch which I plan to be using instead.  There is more
cleanup that could be done, but I assume the reason the reason why you
were interested in this was to reduce ext4's compile time (we have
protection against double inclusion, so it otherwise wouldn't have
made much difference).  So this approach (which I had suggested and
had been hoping you would do :-) is better in that regard...

    	 	    	     	    - Ted

commit 4a092d737955301da22b9d5e07f5036da821a932
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Wed Nov 28 13:03:30 2012 -0500

    ext4: rationalize ext4_extents.h inclusion
    
    Previously, ext4_extents.h was being included at the end of ext4.h,
    which was bad for a number of reasons: (a) it was not being included
    in the expected place, and (b) it caused the header to be included
    multiple times.  There were #ifdef's to prevent this from causing any
    problems, but it still was unnecessary.
    
    By moving the function declarations that were in ext4_extents.h to
    ext4.h, which is standard practice for where the function declarations
    for the rest of ext4.h can be found, we can remove ext4_extents.h from
    being included in ext4.h at all, and then we can only include
    ext4_extents.h where it is needed in ext4's source files.
    
    It should be possible to move a few more things into ext4.h, and
    further reduce the number of source files that need to #include
    ext4_extents.h, but that's a cleanup for another day.
    
    Reported-by: Sachin Kamat <sachin.kamat@linaro.org>
    Reported-by: Wei Yongjun <weiyj.lk@gmail.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 246e38f..2e9ffa9 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -57,6 +57,16 @@
 #define ext4_debug(fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
 #endif
 
+/*
+ * Turn on EXT_DEBUG to get lots of info about extents operations.
+ */
+#define EXT_DEBUG__
+#ifdef EXT_DEBUG
+#define ext_debug(fmt, ...)	printk(fmt, ##__VA_ARGS__)
+#else
+#define ext_debug(fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
+#endif
+
 #define EXT4_ERROR_INODE(inode, fmt, a...) \
 	ext4_error_inode((inode), __func__, __LINE__, 0, (fmt), ## a)
 
@@ -2399,6 +2409,9 @@ extern int ext4_check_blockref(const char *, unsigned int,
 			       struct inode *, __le32 *, unsigned int);
 
 /* extents.c */
+struct ext4_ext_path;
+struct ext4_extent;
+
 extern int ext4_ext_tree_init(handle_t *handle, struct inode *);
 extern int ext4_ext_writepage_trans_blocks(struct inode *, int);
 extern int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks,
@@ -2416,8 +2429,27 @@ extern int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
 			  ssize_t len);
 extern int ext4_map_blocks(handle_t *handle, struct inode *inode,
 			   struct ext4_map_blocks *map, int flags);
+extern int ext4_ext_calc_metadata_amount(struct inode *inode,
+					 ext4_lblk_t lblocks);
+extern int ext4_extent_tree_init(handle_t *, struct inode *);
+extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
+						   int num,
+						   struct ext4_ext_path *path);
+extern int ext4_can_extents_be_merged(struct inode *inode,
+				      struct ext4_extent *ex1,
+				      struct ext4_extent *ex2);
+extern int ext4_ext_insert_extent(handle_t *, struct inode *,
+				  struct ext4_ext_path *,
+				  struct ext4_extent *, int);
+extern struct ext4_ext_path *ext4_ext_find_extent(struct inode *, ext4_lblk_t,
+						  struct ext4_ext_path *);
+extern void ext4_ext_drop_refs(struct ext4_ext_path *);
+extern int ext4_ext_check_inode(struct inode *inode);
+extern int ext4_find_delalloc_cluster(struct inode *inode, ext4_lblk_t lblk);
 extern int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			__u64 start, __u64 len);
+
+
 /* move_extent.c */
 extern int ext4_move_extents(struct file *o_filp, struct file *d_filp,
 			     __u64 start_orig, __u64 start_donor,
@@ -2505,6 +2537,4 @@ extern void ext4_resize_end(struct super_block *sb);
 
 #endif	/* __KERNEL__ */
 
-#include "ext4_extents.h"
-
 #endif	/* _EXT4_H */
diff --git a/fs/ext4/ext4_extents.h b/fs/ext4/ext4_extents.h
index 173b6c5..487fda1 100644
--- a/fs/ext4/ext4_extents.h
+++ b/fs/ext4/ext4_extents.h
@@ -43,16 +43,6 @@
 #define CHECK_BINSEARCH__
 
 /*
- * Turn on EXT_DEBUG to get lots of info about extents operations.
- */
-#define EXT_DEBUG__
-#ifdef EXT_DEBUG
-#define ext_debug(fmt, ...)	printk(fmt, ##__VA_ARGS__)
-#else
-#define ext_debug(fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
-#endif
-
-/*
  * If EXT_STATS is defined then stats numbers are collected.
  * These number will be displayed at umount time.
  */
@@ -286,20 +276,5 @@ static inline void ext4_idx_store_pblock(struct ext4_extent_idx *ix,
 				     0xffff);
 }
 
-extern int ext4_ext_calc_metadata_amount(struct inode *inode,
-					 ext4_lblk_t lblocks);
-extern int ext4_extent_tree_init(handle_t *, struct inode *);
-extern int ext4_ext_calc_credits_for_single_extent(struct inode *inode,
-						   int num,
-						   struct ext4_ext_path *path);
-extern int ext4_can_extents_be_merged(struct inode *inode,
-				      struct ext4_extent *ex1,
-				      struct ext4_extent *ex2);
-extern int ext4_ext_insert_extent(handle_t *, struct inode *, struct ext4_ext_path *, struct ext4_extent *, int);
-extern struct ext4_ext_path *ext4_ext_find_extent(struct inode *, ext4_lblk_t,
-							struct ext4_ext_path *);
-extern void ext4_ext_drop_refs(struct ext4_ext_path *);
-extern int ext4_ext_check_inode(struct inode *inode);
-extern int ext4_find_delalloc_cluster(struct inode *inode, ext4_lblk_t lblk);
 #endif /* _EXT4_EXTENTS */
 
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 5625146..1dc19a7 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -41,6 +41,7 @@
 #include <asm/uaccess.h>
 #include <linux/fiemap.h>
 #include "ext4_jbd2.h"
+#include "ext4_extents.h"
 
 #include <trace/events/ext4.h>
 
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index f6663c3..20862f9 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -22,6 +22,7 @@
 
 #include "ext4_jbd2.h"
 #include "truncate.h"
+#include "ext4_extents.h"	/* Needed for EXT_MAX_BLOCKS */
 
 #include <trace/events/ext4.h>
 
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index f1bb32e..db8226d 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -14,6 +14,7 @@
 
 #include <linux/slab.h>
 #include "ext4_jbd2.h"
+#include "ext4_extents.h"
 
 /*
  * The contiguous blocks details which can be
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 292daee..d9cc5ee 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include "ext4_jbd2.h"
 #include "ext4.h"
+#include "ext4_extents.h"
 
 /**
  * get_ext_path - Find an extent path for designated logical block number.
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 0fd16e6..0016fbc 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -27,7 +27,6 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
-#include "ext4_extents.h"
 
 static struct kmem_cache *io_page_cachep, *io_end_cachep;
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 66a4e20..856206f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -45,12 +45,11 @@
 #include <linux/freezer.h>
 
 #include "ext4.h"
-#include "ext4_extents.h"
+#include "ext4_extents.h"	/* Needed for trace points definition */
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
 #include "mballoc.h"
-#include "ext4_extents.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/ext4.h>