Daniel J Blueman
2010-Jul-25 18:53 UTC
Re: [2.6.35-rc6 patch] direct I/O submission fixes v2
On 25 July 2010 15:42, Josef Bacik <josef@redhat.com> wrote:> On Sat, Jul 24, 2010 at 12:01:59AM +0100, Daniel J Blueman wrote: >> Hi Chris, >> >> This fixes some issues relating to direct I/O submission, however a >> further patch will be needed to handle the case where allocation of >> ''dip'' fails, which is always dereferenced when finding the ordered >> extent. >> > > Hi, > > There''s an easier way to do this. This patch should fix the problem, > > Signed-off-by: Josef Bacik <josef@redhat.com> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 3232945..7259ef9 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -5815,7 +5815,7 @@ free_ordered: > if (write) { > struct btrfs_ordered_extent *ordered; > ordered = btrfs_lookup_ordered_extent(inode, > - dip->logical_offset); > + file_offset); > if (!test_bit(BTRFS_ORDERED_PREALLOC, &ordered->flags) && > !test_bit(BTRFS_ORDERED_NOCOW, &ordered->flags)) > btrfs_free_reserved_extent(root, ordered->start, >Good move! With your patch applied, mine (now not priority) then becomes: Fix leak of ''dip'' on error path and double assignment. Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 1bff92a..bd7f940 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -5652,15 +5652,15 @@ static void btrfs_submit_direct(int rw, struct bio *bio, struct inode *inode, ret = -ENOMEM; goto free_ordered; } - dip->csums = NULL; if (!skip_sum) { dip->csums = kmalloc(sizeof(u32) * bio->bi_vcnt, GFP_NOFS); if (!dip->csums) { ret = -ENOMEM; - goto free_ordered; + goto out_err; } - } + } else + dip->csums = NULL; dip->private = bio->bi_private; dip->inode = inode; -- Daniel J Blueman -- 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
Hi Chris, On 25 July 2010 15:42, Josef Bacik <josef@redhat.com> wrote:> On Sat, Jul 24, 2010 at 12:01:59AM +0100, Daniel J Blueman wrote: >> Hi Chris, >> >> This fixes some issues relating to direct I/O submission, however a >> further patch will be needed to handle the case where allocation of >> ''dip'' fails, which is always dereferenced when finding the ordered >> extent. >> > > Hi, > > There''s an easier way to do this. This patch should fix the problem, > > Signed-off-by: Josef Bacik <josef@redhat.com> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 3232945..7259ef9 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -5815,7 +5815,7 @@ free_ordered: > if (write) { > struct btrfs_ordered_extent *ordered; > ordered = btrfs_lookup_ordered_extent(inode, > - dip->logical_offset); > + file_offset); > if (!test_bit(BTRFS_ORDERED_PREALLOC, &ordered->flags) && > !test_bit(BTRFS_ORDERED_NOCOW, &ordered->flags)) > btrfs_free_reserved_extent(root, ordered->start, >Good move! With your patch applied, mine (now not priority) then becomes: Fix leak of ''dip'' on error path and double assignment. Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 1bff92a..bd7f940 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -5652,15 +5652,15 @@ static void btrfs_submit_direct(int rw, struct bio *bio, struct inode *inode, ret = -ENOMEM; goto free_ordered; } - dip->csums = NULL; if (!skip_sum) { dip->csums = kmalloc(sizeof(u32) * bio->bi_vcnt, GFP_NOFS); if (!dip->csums) { ret = -ENOMEM; - goto free_ordered; + goto out_err; } - } + } else + dip->csums = NULL; dip->private = bio->bi_private; dip->inode = inode; I didn''t see Josef and my patches at http://git.kernel.org/?p=linux/kernel/git/mason/btrfs-unstable.git yet. They still appear relevant; let me know if you''d like it rediffed against your tree. Thanks, Daniel -- Daniel J Blueman -- 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