Re: [PATCH] btrfs: make static code static remove dead code
On Mon, Apr 22, 2013 at 11:01:09AM -0500, Eric Sandeen wrote: Maybe one for Arne to answer? Yeah, I don't see it in the manpage or in userspace, so *shrug* where is the design? Scrub can be started on one device, cancel is just part of the command set. $ ./btrfs scrub cancel --help usage: btrfs scrub cancel path|device Cancel a running scrub Right, but device is different from devid, no? i.e. btrfs replace start can take a devid or a device, and checks for if (is_numerical(srcdev)); this doesn't look implemented for scrub cancel. I guess it could be. Yeah, after further research, the 'scrub cancel' ioctl uses the device just to find the filesystem as if a path were given, and does not cancel scrub on that device. The ioctl does not take any user data so we'd have to do IOC_SCRUB_CANCEL_V2 in order to implement cancel-by-id (or delete the function if this is not a valid usecase, I don't know). david -- 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] btrfs: make static code static remove dead code
On Fri, Apr 19, 2013 at 12:21:22PM -0700, Eric Sandeen wrote: Big patch, but all it does is add statics to functions which are in fact static, then remove the associated dead-code fallout. I support removing dead code, though nearly all of the functions make me wonder why they're not used. A few samples I see immediatelly: btrfs_iref_to_path() The removed comment looks useful, I'd rather see it transferred to a function with similar goal (that probably got a different name during time). __btrfs_lookup_delayed_deletion_item() __btrfs_search_delayed_insertion_item() __btrfs_search_delayed_deletion_item() find_eb_for_page() btrfs_find_block_group() range_straddles_pages() extent_range_uptodate() btrfs_file_extent_length() btrfs_reada_detach() That's an API for readahead, thhugh maybe not used now as RA is not used and at all scenarios where it could. btrfs_scrub_cancel_devid() Looks like there's a missing userspace counterpart, cancelling scrub by device is possible by design. btrfs_start_transaction_lflush() Transcaction API, removing the func does not make sense without removing BTRFS_RESERVE_FLUSH_LIMIT at the same time. david -- 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] btrfs: make static code static remove dead code
On 4/22/13 10:05 AM, David Sterba wrote: On Fri, Apr 19, 2013 at 12:21:22PM -0700, Eric Sandeen wrote: Big patch, but all it does is add statics to functions which are in fact static, then remove the associated dead-code fallout. I support removing dead code, though nearly all of the functions make me wonder why they're not used. That's still a little above my btrfs pay grade. ;) A few samples I see immediatelly: btrfs_iref_to_path() The removed comment looks useful, I'd rather see it transferred to a function with similar goal (that probably got a different name during time). whoops, yep, it probably belongs on btrfs_ref_to_path(), sorry about that. __btrfs_lookup_delayed_deletion_item() __btrfs_search_delayed_insertion_item() __btrfs_search_delayed_deletion_item() find_eb_for_page() btrfs_find_block_group() range_straddles_pages() extent_range_uptodate() btrfs_file_extent_length() btrfs_reada_detach() That's an API for readahead, thhugh maybe not used now as RA is not used and at all scenarios where it could. Ok, if it's useful to keep it around just for symmetry I could understand that. btrfs_scrub_cancel_devid() Looks like there's a missing userspace counterpart, cancelling scrub by device is possible by design. Maybe one for Arne to answer? Yeah, I don't see it in the manpage or in userspace, so *shrug* where is the design? btrfs_start_transaction_lflush() Transcaction API, removing the func does not make sense without removing BTRFS_RESERVE_FLUSH_LIMIT at the same time. Not sure I understand that, btrfs_block_rsv_refill() still uses that macro, but I'm probably not understanding the code or missing your point. I'll admit to doing the removal mechanically, hopefully those with particular affinity to any of the removed functions can speak up. :) Thanks, -Eric david -- 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] btrfs: make static code static remove dead code
On Mon, Apr 22, 2013 at 10:24:12AM -0500, Eric Sandeen wrote: btrfs_reada_detach() That's an API for readahead, thhugh maybe not used now as RA is not used and at all scenarios where it could. Ok, if it's useful to keep it around just for symmetry I could understand that. reada.c: /* * This is the implementation for the generic read ahead framework. * * To trigger a readahead, btrfs_reada_add must be called. It will start * a read ahead for the given range [start, end) on tree root. The returned * handle can either be used to wait on the readahead to finish * (btrfs_reada_wait), or to send it to the background (btrfs_reada_detach). btrfs_scrub_cancel_devid() Looks like there's a missing userspace counterpart, cancelling scrub by device is possible by design. Maybe one for Arne to answer? Yeah, I don't see it in the manpage or in userspace, so *shrug* where is the design? Scrub can be started on one device, cancel is just part of the command set. $ ./btrfs scrub cancel --help usage: btrfs scrub cancel path|device Cancel a running scrub btrfs_start_transaction_lflush() Transcaction API, removing the func does not make sense without removing BTRFS_RESERVE_FLUSH_LIMIT at the same time. Not sure I understand that, btrfs_block_rsv_refill() still uses that macro, but I'm probably not understanding the code or missing your point. The function wraps usage of the macro/enum in the transaction_start_* functions and it has been used at some point, we may want to use it again as allows to start a transaction in some limited context. It's been added by Miao, I'll admit to doing the removal mechanically, hopefully those with particular affinity to any of the removed functions can speak up. :) Removing dead code sometimes points out bugs, it's always good to do a quick review and do patch archeology first. Which makes it less appealing as a low-hanging fruit :) david -- 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] btrfs: make static code static remove dead code
On 4/22/13 10:55 AM, David Sterba wrote: On Mon, Apr 22, 2013 at 10:24:12AM -0500, Eric Sandeen wrote: btrfs_reada_detach() That's an API for readahead, thhugh maybe not used now as RA is not used and at all scenarios where it could. Ok, if it's useful to keep it around just for symmetry I could understand that. reada.c: /* * This is the implementation for the generic read ahead framework. * * To trigger a readahead, btrfs_reada_add must be called. It will start * a read ahead for the given range [start, end) on tree root. The returned * handle can either be used to wait on the readahead to finish * (btrfs_reada_wait), or to send it to the background (btrfs_reada_detach). btrfs_scrub_cancel_devid() Looks like there's a missing userspace counterpart, cancelling scrub by device is possible by design. Maybe one for Arne to answer? Yeah, I don't see it in the manpage or in userspace, so *shrug* where is the design? Scrub can be started on one device, cancel is just part of the command set. $ ./btrfs scrub cancel --help usage: btrfs scrub cancel path|device Cancel a running scrub Right, but device is different from devid, no? i.e. btrfs replace start can take a devid or a device, and checks for if (is_numerical(srcdev)); this doesn't look implemented for scrub cancel. I guess it could be. btrfs_start_transaction_lflush() Transcaction API, removing the func does not make sense without removing BTRFS_RESERVE_FLUSH_LIMIT at the same time. Not sure I understand that, btrfs_block_rsv_refill() still uses that macro, but I'm probably not understanding the code or missing your point. The function wraps usage of the macro/enum in the transaction_start_* functions and it has been used at some point, we may want to use it again as allows to start a transaction in some limited context. It's been added by Miao, I'll admit to doing the removal mechanically, hopefully those with particular affinity to any of the removed functions can speak up. :) Removing dead code sometimes points out bugs, it's always good to do a quick review and do patch archeology first. Which makes it less appealing as a low-hanging fruit :) Fair enough, I'm appropriately chastised. :) -Eric david -- 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] btrfs: make static code static remove dead code
Hello Eric, Big patch, but all it does is add statics to functions which are in fact static, then remove the associated dead-code fallout. removed functions: btrfs_iref_to_path() __btrfs_lookup_delayed_deletion_item() __btrfs_search_delayed_insertion_item() __btrfs_search_delayed_deletion_item() find_eb_for_page() btrfs_find_block_group() range_straddles_pages() extent_range_uptodate() btrfs_file_extent_length() btrfs_reada_detach() btrfs_scrub_cancel_devid() btrfs_start_transaction_lflush() btrfs_print_tree() is left because it is used for debugging. ulist.c functions are left because that one is odd; they're exported? I have sent another patch regarding to ulist.c functions….. Thanks, Wang. Signed-off-by: Eric Sandeen sand...@redhat.com 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 -- 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] btrfs: make static code static remove dead code
On 04/19/13 21:21, Eric Sandeen wrote:
Big patch, but all it does is add statics to functions which
are in fact static, then remove the associated dead-code fallout.
removed functions:
btrfs_iref_to_path()
__btrfs_lookup_delayed_deletion_item()
__btrfs_search_delayed_insertion_item()
__btrfs_search_delayed_deletion_item()
find_eb_for_page()
btrfs_find_block_group()
range_straddles_pages()
extent_range_uptodate()
btrfs_file_extent_length()
btrfs_reada_detach()
btrfs_scrub_cancel_devid()
btrfs_start_transaction_lflush()
btrfs_print_tree() is left because it is used for debugging.
ulist.c functions are left because that one is odd; they're exported?
I originally wrote them to be included under lib/, but got no response
on LKML, so I added it only to btrfs, but forgot to remove the exports
for that.
I think it would still make sense to move them to lib/, as this data
structure can be quite useful from time to time.
-Arne
Signed-off-by: Eric Sandeen sand...@redhat.com
---
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index bd605c8..cb8acab 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1249,32 +1249,6 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
}
/*
- * this iterates to turn a btrfs_inode_ref into a full filesystem path.
elements
- * of the path are separated by '/' and the path is guaranteed to be
- * 0-terminated. the path is only given within the current file system.
- * Therefore, it never starts with a '/'. the caller is responsible to
provide
- * size bytes in dest. the dest buffer will be filled backwards. finally,
- * the start point of the resulting string is returned. this pointer is
within
- * dest, normally.
- * in case the path buffer would overflow, the pointer is decremented further
- * as if output was written to the buffer, though no more output is actually
- * generated. that way, the caller can determine how much space would be
- * required for the path to fit into the buffer. in that case, the returned
- * value will be smaller than dest. callers must check this!
- */
-char *btrfs_iref_to_path(struct btrfs_root *fs_root,
- struct btrfs_path *path,
- struct btrfs_inode_ref *iref,
- struct extent_buffer *eb_in, u64 parent,
- char *dest, u32 size)
-{
- return btrfs_ref_to_path(fs_root, path,
- btrfs_inode_ref_name_len(eb_in, iref),
- (unsigned long)(iref + 1),
- eb_in, parent, dest, size);
-}
-
-/*
* this makes the path point to (logical EXTENT_ITEM *)
* returns BTRFS_EXTENT_FLAG_DATA for data, BTRFS_EXTENT_FLAG_TREE_BLOCK for
* tree blocks and 0 on error.
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 310a7f6..0f446d7 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -59,9 +59,6 @@ int paths_from_inode(u64 inum, struct inode_fs_paths
*ipath);
int btrfs_find_all_roots(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 bytenr,
u64 time_seq, struct ulist **roots);
-char *btrfs_iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
- struct btrfs_inode_ref *iref, struct extent_buffer *eb,
- u64 parent, char *dest, u32 size);
char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
u32 name_len, unsigned long name_off,
struct extent_buffer *eb_in, u64 parent,
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 15b9408..e139bbf 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -82,6 +82,10 @@ struct compressed_bio {
u32 sums;
};
+static int btrfs_decompress_biovec(int type, struct page **pages_in,
+u64 disk_start, struct bio_vec *bvec,
+int vcnt, size_t srclen);
+
static inline int compressed_bio_size(struct btrfs_root *root,
unsigned long disk_size)
{
@@ -739,7 +743,7 @@ static int comp_num_workspace[BTRFS_COMPRESS_TYPES];
static atomic_t comp_alloc_workspace[BTRFS_COMPRESS_TYPES];
static wait_queue_head_t comp_workspace_wait[BTRFS_COMPRESS_TYPES];
-struct btrfs_compress_op *btrfs_compress_op[] = {
+static struct btrfs_compress_op *btrfs_compress_op[] = {
btrfs_zlib_compress,
btrfs_lzo_compress,
};
@@ -910,8 +914,9 @@ int btrfs_compress_pages(int type, struct address_space
*mapping,
* be contiguous. They all correspond to the range of bytes covered by
* the compressed extent.
*/
-int btrfs_decompress_biovec(int type, struct page **pages_in, u64 disk_start,
- struct bio_vec *bvec, int vcnt, size_t srclen)
+static int
[PATCH] btrfs: make static code static remove dead code
Big patch, but all it does is add statics to functions which
are in fact static, then remove the associated dead-code fallout.
removed functions:
btrfs_iref_to_path()
__btrfs_lookup_delayed_deletion_item()
__btrfs_search_delayed_insertion_item()
__btrfs_search_delayed_deletion_item()
find_eb_for_page()
btrfs_find_block_group()
range_straddles_pages()
extent_range_uptodate()
btrfs_file_extent_length()
btrfs_reada_detach()
btrfs_scrub_cancel_devid()
btrfs_start_transaction_lflush()
btrfs_print_tree() is left because it is used for debugging.
ulist.c functions are left because that one is odd; they're exported?
Signed-off-by: Eric Sandeen sand...@redhat.com
---
diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index bd605c8..cb8acab 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1249,32 +1249,6 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root,
struct btrfs_path *path,
}
/*
- * this iterates to turn a btrfs_inode_ref into a full filesystem path.
elements
- * of the path are separated by '/' and the path is guaranteed to be
- * 0-terminated. the path is only given within the current file system.
- * Therefore, it never starts with a '/'. the caller is responsible to provide
- * size bytes in dest. the dest buffer will be filled backwards. finally,
- * the start point of the resulting string is returned. this pointer is within
- * dest, normally.
- * in case the path buffer would overflow, the pointer is decremented further
- * as if output was written to the buffer, though no more output is actually
- * generated. that way, the caller can determine how much space would be
- * required for the path to fit into the buffer. in that case, the returned
- * value will be smaller than dest. callers must check this!
- */
-char *btrfs_iref_to_path(struct btrfs_root *fs_root,
-struct btrfs_path *path,
-struct btrfs_inode_ref *iref,
-struct extent_buffer *eb_in, u64 parent,
-char *dest, u32 size)
-{
- return btrfs_ref_to_path(fs_root, path,
-btrfs_inode_ref_name_len(eb_in, iref),
-(unsigned long)(iref + 1),
-eb_in, parent, dest, size);
-}
-
-/*
* this makes the path point to (logical EXTENT_ITEM *)
* returns BTRFS_EXTENT_FLAG_DATA for data, BTRFS_EXTENT_FLAG_TREE_BLOCK for
* tree blocks and 0 on error.
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
index 310a7f6..0f446d7 100644
--- a/fs/btrfs/backref.h
+++ b/fs/btrfs/backref.h
@@ -59,9 +59,6 @@ int paths_from_inode(u64 inum, struct inode_fs_paths *ipath);
int btrfs_find_all_roots(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 bytenr,
u64 time_seq, struct ulist **roots);
-char *btrfs_iref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
-struct btrfs_inode_ref *iref, struct extent_buffer *eb,
-u64 parent, char *dest, u32 size);
char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
u32 name_len, unsigned long name_off,
struct extent_buffer *eb_in, u64 parent,
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 15b9408..e139bbf 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -82,6 +82,10 @@ struct compressed_bio {
u32 sums;
};
+static int btrfs_decompress_biovec(int type, struct page **pages_in,
+ u64 disk_start, struct bio_vec *bvec,
+ int vcnt, size_t srclen);
+
static inline int compressed_bio_size(struct btrfs_root *root,
unsigned long disk_size)
{
@@ -739,7 +743,7 @@ static int comp_num_workspace[BTRFS_COMPRESS_TYPES];
static atomic_t comp_alloc_workspace[BTRFS_COMPRESS_TYPES];
static wait_queue_head_t comp_workspace_wait[BTRFS_COMPRESS_TYPES];
-struct btrfs_compress_op *btrfs_compress_op[] = {
+static struct btrfs_compress_op *btrfs_compress_op[] = {
btrfs_zlib_compress,
btrfs_lzo_compress,
};
@@ -910,8 +914,9 @@ int btrfs_compress_pages(int type, struct address_space
*mapping,
* be contiguous. They all correspond to the range of bytes covered by
* the compressed extent.
*/
-int btrfs_decompress_biovec(int type, struct page **pages_in, u64 disk_start,
- struct bio_vec *bvec, int vcnt, size_t srclen)
+static int btrfs_decompress_biovec(int type, struct page **pages_in,
+ u64 disk_start, struct bio_vec *bvec,
+ int vcnt, size_t srclen)
{
struct list_head *workspace;
int ret;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 9afb0a6..0c803b4 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -30,8
Re: [PATCH] btrfs: make static code static remove dead code
Hello Eric, Big patch, but all it does is add statics to functions which are in fact static, then remove the associated dead-code fallout. removed functions: btrfs_iref_to_path() __btrfs_lookup_delayed_deletion_item() __btrfs_search_delayed_insertion_item() __btrfs_search_delayed_deletion_item() find_eb_for_page() btrfs_find_block_group() range_straddles_pages() extent_range_uptodate() btrfs_file_extent_length() btrfs_reada_detach() btrfs_scrub_cancel_devid() btrfs_start_transaction_lflush() btrfs_print_tree() is left because it is used for debugging. ulist.c functions are left because that one is odd; they're exported? ulist_fin() and ulist_init() are only used static in fact, i don't think they they should be exported as symbol. So you can just remove exporting and make them static. Thanks, Wang Signed-off-by: Eric Sandeen sand...@redhat.com --- [snip] 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 -- 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
