From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fan Du Subject: Re: [PATCH] fs: Disable preempt when acquire i_size_seqcount write lock Date: Fri, 11 Jan 2013 11:25:08 +0800 Message-ID: <50EF8614.3050408@windriver.com> References: <1357702459-2718-1-git-send-email-fan.du@windriver.com> <20130110143813.1ba2b4fd.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , To: Andrew Morton Return-path: In-Reply-To: <20130110143813.1ba2b4fd.akpm@linux-foundation.org> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On 2013=E5=B9=B401=E6=9C=8811=E6=97=A5 06:38, Andrew Morton wrote: > On Wed, 9 Jan 2013 11:34:19 +0800 > Fan Du wrote: > >> Two rt tasks bind to one CPU core. >> >> The higher priority rt task A preempts a lower priority rt task B wh= ich >> has already taken the write seq lock, and then the higher priority >> rt task A try to acquire read seq lock, it's doomed to lockup. >> >> rt task A with lower priority: call write >> i_size_write rt task B with h= igher priority: call sync, and preempt task A >> write_seqcount_begin(&inode->i_size_seqcount); i_size_read >> inode->i_size =3D i_size; read_seqcou= nt_begin<-- lockup here... >> > > Ouch. > > And even if the preemping task is preemptible, it will spend an entir= e > timeslice pointlessly spinning, which isn't very good. > >> So disable preempt when acquiring every i_size_seqcount *write* lock= will >> cure the problem. >> >> ... >> >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -758,9 +758,11 @@ static inline loff_t i_size_read(const struct i= node *inode) >> static inline void i_size_write(struct inode *inode, loff_t i_size= ) >> { >> #if BITS_PER_LONG=3D=3D32&& defined(CONFIG_SMP) >> + preempt_disable(); >> write_seqcount_begin(&inode->i_size_seqcount); >> inode->i_size =3D i_size; >> write_seqcount_end(&inode->i_size_seqcount); >> + preempt_enable(); >> #elif BITS_PER_LONG=3D=3D32&& defined(CONFIG_PREEMPT) >> preempt_disable(); >> inode->i_size =3D i_size; > > afacit all write_seqcount_begin()/read_seqretry() sites are vulnerabl= e > to this problem. Would it not be better to do the preempt_disable() = in > write_seqcount_begin()? IMHO, write_seqcount_begin/write_seqcount_end are often wrapped by mute= x, this gives higher priority task a chance to sleep, and then lower prior= ity task get cpu to unlock, so avoid the problematic scenario this patch describ= ing. But in i_size_write case, I could only find disable preempt a good choi= ce before someone else has better idea :) > > Possible problems: > > - mm/filemap_xip.c does disk I/O under write_seqcount_begin(). > > - dev_change_name() does GFP_KERNEL allocations under write_seqcount_= begin() > > - I didn't review u64_stats_update_begin() callers. > > But I think calling schedule() under preempt_disable() is OK anyway? > --=20 =E6=B5=AE=E6=B2=89=E9=9A=8F=E6=B5=AA=E5=8F=AA=E8=AE=B0=E4=BB=8A=E6=9C=9D= =E7=AC=91 --fan