| Message ID | 1433323794-10045-1-git-send-email-chandan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jun 03, 2015 at 02:59:54PM +0530, Chandan Rajendra wrote: > max_to_defrag represents the number of pages to defrag rather than the last > page of the file range to be defragged. Fix this. Please update the changelog and describe the buggy behaviour. From a brief look I think that the defrag batch will be much higher than desired. Elsewhere the defrag batch is 1024, the page indexes used here are always large numbers. I guess this could (partially) explain high load during defrag that people report. ACK for the fix, I'll stick a reviewed-by to v2. > > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> > --- > fs/btrfs/ioctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index ca5d968..2a45026 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -1318,7 +1318,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, > i = range->start >> PAGE_CACHE_SHIFT; > } > if (!max_to_defrag) > - max_to_defrag = last_index + 1; > + max_to_defrag = last_index - i + 1; > > /* > * make writeback starts from i, so the defrag range can be > -- > 2.1.0 > > -- > 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 -- 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 Wednesday 03 Jun 2015 12:41:02 David Sterba wrote: > On Wed, Jun 03, 2015 at 02:59:54PM +0530, Chandan Rajendra wrote: > > max_to_defrag represents the number of pages to defrag rather than the > > last > > page of the file range to be defragged. Fix this. > > Please update the changelog and describe the buggy behaviour. From a > brief look I think that the defrag batch will be much higher than > desired. Elsewhere the defrag batch is 1024, the page indexes used here > are always large numbers. I guess this could (partially) explain high > load during defrag that people report. > > ACK for the fix, I'll stick a reviewed-by to v2. Hello Dave, Consider a file having 10 4k blocks (i.e. blocks in the range [0 - 9]). If the defrag ioctl was invoked for the block range [3 - 6], then max_to_defrag should actually have the value 4. Instead in the current code we end up setting it to 6. Now, this does not (yet) cause an issue since the first part of the while loop condition in btrfs_defrag_file() (i.e. "i <= last_index") causes the control to flow out of the while loop before any buggy behavior is actually caused. So the patch just makes sure that max_to_defrag ends up having the right value rather than fixing a bug. I did run the xfstests suite to make sure that no new regressions were introduced by this patch.
On Wed, Jun 03, 2015 at 06:16:23PM +0530, Chandan Rajendra wrote: > Consider a file having 10 4k blocks (i.e. blocks in the range [0 - 9]). If the > defrag ioctl was invoked for the block range [3 - 6], then max_to_defrag > should actually have the value 4. Instead in the current code we end up > setting it to 6. > > Now, this does not (yet) cause an issue since the first part of the while loop > condition in btrfs_defrag_file() (i.e. "i <= last_index") causes the control > to flow out of the while loop before any buggy behavior is actually caused. So > the patch just makes sure that max_to_defrag ends up having the right value > rather than fixing a bug. I did run the xfstests suite to make sure that no > new regressions were introduced by this patch. Thank, put that kind of detailed description into the changelog. -- 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/ioctl.c b/fs/btrfs/ioctl.c index ca5d968..2a45026 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1318,7 +1318,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, i = range->start >> PAGE_CACHE_SHIFT; } if (!max_to_defrag) - max_to_defrag = last_index + 1; + max_to_defrag = last_index - i + 1; /* * make writeback starts from i, so the defrag range can be
max_to_defrag represents the number of pages to defrag rather than the last page of the file range to be defragged. Fix this. Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com> --- fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)