On Thu 07-06-18 16:47:18, Eric Whitney wrote:
> * Jan Kara <jack@xxxxxxx>:
> > On Sun 13-05-18 13:56:20, Eric Whitney wrote:
> > > The code in ext4_da_map_blocks sometimes reserves space for more
> > > delayed allocated clusters than it should, resulting in premature
> > > ENOSPC, exceeded quota, and inaccurate free space reporting.
> > >
> > > It fails to check the extents status tree for written and unwritten
> > > clusters which have already been allocated and which share a cluster
> > > with the block being delayed allocated. In addition, it fails to
> > > continue on and search the extent tree when no delayed allocated or
> > > allocated clusters have been found in the extents status tree. Written
> > > extents may be purged from the extents status tree under memory pressure.
> > > Only delayed and unwritten delayed extents are guaranteed to be retained.
> > >
> > > Signed-off-by: Eric Whitney <enwlinux@xxxxxxxxx>
> >
> > Thanks for the patch Eric. Some comments are below.
> >
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index c969275ce3ee..872782ba8bc3 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -3846,6 +3846,47 @@ int ext4_find_delalloc_cluster(struct inode *inode, ext4_lblk_t lblk)
> > > return ext4_find_delalloc_range(inode, lblk_start, lblk_end);
> > > }
> > >
> > > +/*
> > > + * If the block range specified by @start and @end contains an es extent
> > > + * matched by the matching function specified by @match_fn, return 1. Else,
> > > + * return 0.
> > > + */
> > > +int ext4_find_range(struct inode *inode,
> > > + int (*match_fn)(struct extent_status *es),
> > > + ext4_lblk_t start,
> > > + ext4_lblk_t end)
> > > +{
> > > + struct extent_status es;
> > > +
> > > + ext4_es_find_extent_range(inode, match_fn, start, end, &es);
> > > + if (es.es_len == 0)
> > > + return 0; /* there is no matching extent in this tree */
> > > + else if (es.es_lblk <= start &&
> > > + start < es.es_lblk + es.es_len)
> > > + return 1;
> > > + else if (start <= es.es_lblk && es.es_lblk <= end)
> > > + return 1;
> > > + else
> > > + return 0;
> >
> > No need to elses here. Also how could is possibly happen that es.es_len > 0
> > and we don't have an extent we want? That would be a bug in
> > ext4_es_find_extent_range(), wouldn't it?
>
> Hi, Jan:
>
> Yes, I just cloned ext4_find_delalloc_range() here without looking too
> critically at it. I'll clean this up.
>
> I think we can get an out of range extent here given the implementation
> of __es_tree_search(), called by ext4_es_find_extent_range(). If "start"
> isn't contained in an extent in the extents status tree, we could get back
> an extent containing block numbers bigger than "start". Rejecting a match
Ah, correct. But then I find it surprising (if not an outright bug) that
ext4_es_find_extent_range() (and ext4_es_find_delayed_extent_range() as
well) can return an extent that's not overlapping given range. IMO it
should just refrain from returning anything in that case (just as it would
if it got to that situation by iterating through extents not maching the
match_fn()). Can you please fix that? Thanks!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
