Messages in this thread | | | From | "Hatayama, Daisuke" <> | Subject | RE: [RESEND PATCH v2] kernfs: fix dentry unexpected skip | Date | Mon, 4 Jun 2018 09:46:17 +0000 |
| |
> -----Original Message----- > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of 'tj@kernel.org' > Sent: Saturday, June 2, 2018 2:07 AM > To: Hatayama, Daisuke <d.hatayama@jp.fujitsu.com> > Cc: 'gregkh@linuxfoundation.org' <gregkh@linuxfoundation.org>; Okajima, > Toshiyuki <toshi.okajima@jp.fujitsu.com>; > linux-kernel@vger.kernel.org; 'ebiederm@aristanetworks.com' > <ebiederm@aristanetworks.com> > Subject: Re: [RESEND PATCH v2] kernfs: fix dentry unexpected skip > > Hello, > > On Fri, Jun 01, 2018 at 09:25:32AM +0000, Hatayama, Daisuke wrote: > > kernfs_dir_pos() checks if a kernfs_node object given as one of its > > arguments is still active and if so returns it, or returns a > > kernfs_node object that is most equal (possibly smaller and larger) to > > the given object. > > Sometimes they're duplicate operations tho, which is exactly the bug > the posted patch is trying to fix. What I'm suggesting is instead of > leaving both instances and skipping one conditionally, put them in one > place and trigger only when necessary. The sequence of operations > would be exactly the same. The only difference is how the code is > organized. >
I see and I think Eric's patch is written as you suggest very well.
> > kernfs_dir_next_pos() returns a kernfs_node object that is next to the > > object given by kernfs_dir_pos(). > > > > Two functions does different things and both need to skip inactive > > nodes. I don't think it natural to remove the skip only from > > kernfs_dir_pos(). > > > > OTOH, throughout getdents(), there is no case that the kernfs_node > > object given to kernfs_dir_pos() is used afterwards in the > > processing. So, is it enough to provide kernfs_dir_next_pos() only? > > Then, the skip code is now not duplicated. > > > > The patch below is my thought. How do you think? > > > > But note that this patch has some bug so that system boot get hang > > without detecting root filesystem disk :) I'm debugging this now. > > I haven't looked into the code that closely but given that we had > cases where both skippings were fine and not, the condition is likely > gonna be a bit tricker?
I agree to this version looks a bit tricker. But I think once the skipping code is separated as Eric's patch, it would be resolved naturally.
> > Thanks. > > -- > tejun >
| |