RE: [f2fs-dev] [PATCH 6/6] f2fs: avoid double lock for cp_rwsem

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> Sent: Tuesday, December 23, 2014 3:41 PM
> To: Chao Yu
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [f2fs-dev] [PATCH 6/6] f2fs: avoid double lock for cp_rwsem
> 
> Hi,
> 
> On Tue, Dec 23, 2014 at 03:01:36PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> > > Sent: Monday, December 22, 2014 12:35 AM
> > > To: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> > > linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 6/6] f2fs: avoid double lock for cp_rwsem
> > >
> > > The __f2fs_add_link is covered by cp_rwsem all the time.
> > > This calls init_inode_metadata, which conducts some acl operations including
> > > memory allocation with GFP_KERNEL previously.
> > > But, under memory pressure, f2fs_write_data_page can be called, which also
> > > grabs cp_mutex too.
> >
> > grabs cp_rwsem.
> 
> Got it.
> 
> >
> > > Basically, it's safe since down_read was used in both of cases, but it'd
> > > better avoid this situation in advance.
> >
> > If checkpoint intend to hold down_write in the middle of the two down_read
> > caller, it will cause a deadlock, so it's not safe.
> 
> What I meant was like this.
> - down_read

If someone else intends to hold down_write (in checkpoint()) here,
we will encounter deadlock.

Because this writer will add itself to inner waiter list of spinlock as there
is an exist reader had held this lock, then the following reader will start to
spin as it detect that waiter list is not empty. Then deadlock occurred.

I guess this design can effectively avoid starve for the writer when encounters
a large number of readers.

Thanks

>   - down_read
>   - up_read
> - up_read
> 
> This should be safe, right?
> Thanks,
> 
> >
> > Could you update the comments?
> >
> > >
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> >
> > Anyway, Nice catch and please add:
> >
> > Reviewed-by: Chao Yu <chao2.yu@xxxxxxxxxxx>
> >
> > > ---
> > >  fs/f2fs/acl.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> > > index 1ccb26b..7f12d28 100644
> > > --- a/fs/f2fs/acl.c
> > > +++ b/fs/f2fs/acl.c
> > > @@ -62,7 +62,7 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t
> size)
> > >  	if (count == 0)
> > >  		return NULL;
> > >
> > > -	acl = posix_acl_alloc(count, GFP_KERNEL);
> > > +	acl = posix_acl_alloc(count, GFP_NOFS);
> > >  	if (!acl)
> > >  		return ERR_PTR(-ENOMEM);
> > >
> > > @@ -116,7 +116,7 @@ static void *f2fs_acl_to_disk(const struct posix_acl *acl, size_t *size)
> > >  	int i;
> > >
> > >  	f2fs_acl = kmalloc(sizeof(struct f2fs_acl_header) + acl->a_count *
> > > -			sizeof(struct f2fs_acl_entry), GFP_KERNEL);
> > > +			sizeof(struct f2fs_acl_entry), GFP_NOFS);
> > >  	if (!f2fs_acl)
> > >  		return ERR_PTR(-ENOMEM);
> > >
> > > --
> > > 2.1.1
> > >
> > >
> > > ------------------------------------------------------------------------------
> > > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> > > from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> > > with Interactivity, Sharing, Native Excel Exports, App Integration & more
> > > Get technology previously reserved for billion-dollar corporations, FREE
> > > http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
> > > _______________________________________________
> > > Linux-f2fs-devel mailing list
> > > Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/