All of lore.kernel.org
 help / color / mirror / Atom feed
From: "J. Bruce Fields" <bfields@fieldses.org>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Neil Brown <neilb@suse.de>, Christoph Hellwig <hch@infradead.org>,
	linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-mtd@lists.infradead.org
Subject: Re: [RFC] Reinstate NFS exportability for JFFS2.
Date: Fri, 2 May 2008 10:08:04 -0400	[thread overview]
Message-ID: <20080502140804.GA16959@fieldses.org> (raw)
In-Reply-To: <1209728238.25560.686.camel@pmac.infradead.org>

On Fri, May 02, 2008 at 12:37:18PM +0100, David Woodhouse wrote:
> On Fri, 2008-05-02 at 11:38 +1000, Neil Brown wrote:
> > Why is there a deadlock here?
> 
> Many file systems have their own locking, and lookup() can end up trying
> to re-take a lock which readdir() is already holding. In the JFFS2 case,
> it's the fs-internal inode mutex, which is required because the garbage
> collector can't use i_mutex for lock ordering reasons.
> 
> See also the readdir implementation and surrounding comments in
> fs/xfs/linux-2.6/xfs_file.c -- and the way GFS2 uses
> gfs2_glock_is_locked_by_me() to avoid the deadlock.
> 
> The annoying thing is that JFFS2 doesn't even _implement_ i_generation,
> so you get no more useful information out of the lookup() call anyway :)
> 
> > Both readdir and lookup are called with i_mutex held on the directory
> > so there should need to do any extra locking (he said, naively).  In
> > the readdirplus cases, i_mutex is held across both the readdir and the
> > lookup....
> > 
> > One problem with your proposed solution is that filehandles aren't all
> > the same length, so you cannot reliably leave space for them.
> 
> Not without moving stuff around during the postprocessing, I suppose.
> Which isn't very pretty -- but it's prettier than some of the hacks we
> have at the moment to avoid the deadlock.

This comes up periodically.  It would be great to finally get it fixed.
The last conversation I remember started at about:

	http://marc.info/?l=linux-kernel&m=119506226209056&w=2

Summary: one approach would be to define a ->readdirplus() that passes a
dentry to its equivalent of the ->filldir callback.  (Christoph points
out that returning a stat struct would be simpler.  But unfortunately we
need to check for mountpoints here, so that's not sufficient.)

--b.

WARNING: multiple messages have this Message-ID (diff)
From: "J. Bruce Fields" <bfields@fieldses.org>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Neil Brown <neilb@suse.de>, Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org,
	linux-nfs@vger.kernel.org
Subject: Re: [RFC] Reinstate NFS exportability for JFFS2.
Date: Fri, 2 May 2008 10:08:04 -0400	[thread overview]
Message-ID: <20080502140804.GA16959@fieldses.org> (raw)
In-Reply-To: <1209728238.25560.686.camel-ZP4jZrcIevRpWr+L1FloEB2eb7JE58TQ@public.gmane.org>

On Fri, May 02, 2008 at 12:37:18PM +0100, David Woodhouse wrote:
> On Fri, 2008-05-02 at 11:38 +1000, Neil Brown wrote:
> > Why is there a deadlock here?
> 
> Many file systems have their own locking, and lookup() can end up trying
> to re-take a lock which readdir() is already holding. In the JFFS2 case,
> it's the fs-internal inode mutex, which is required because the garbage
> collector can't use i_mutex for lock ordering reasons.
> 
> See also the readdir implementation and surrounding comments in
> fs/xfs/linux-2.6/xfs_file.c -- and the way GFS2 uses
> gfs2_glock_is_locked_by_me() to avoid the deadlock.
> 
> The annoying thing is that JFFS2 doesn't even _implement_ i_generation,
> so you get no more useful information out of the lookup() call anyway :)
> 
> > Both readdir and lookup are called with i_mutex held on the directory
> > so there should need to do any extra locking (he said, naively).  In
> > the readdirplus cases, i_mutex is held across both the readdir and the
> > lookup....
> > 
> > One problem with your proposed solution is that filehandles aren't all
> > the same length, so you cannot reliably leave space for them.
> 
> Not without moving stuff around during the postprocessing, I suppose.
> Which isn't very pretty -- but it's prettier than some of the hacks we
> have at the moment to avoid the deadlock.

This comes up periodically.  It would be great to finally get it fixed.
The last conversation I remember started at about:

	http://marc.info/?l=linux-kernel&m=119506226209056&w=2

Summary: one approach would be to define a ->readdirplus() that passes a
dentry to its equivalent of the ->filldir callback.  (Christoph points
out that returning a stat struct would be simpler.  But unfortunately we
need to check for mountpoints here, so that's not sufficient.)

--b.

WARNING: multiple messages have this Message-ID (diff)
From: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
To: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Neil Brown <neilb-l3A5Bk7waGM@public.gmane.org>,
	Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>,
	linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [RFC] Reinstate NFS exportability for JFFS2.
Date: Fri, 2 May 2008 10:08:04 -0400	[thread overview]
Message-ID: <20080502140804.GA16959@fieldses.org> (raw)
In-Reply-To: <1209728238.25560.686.camel-ZP4jZrcIevRpWr+L1FloEB2eb7JE58TQ@public.gmane.org>

On Fri, May 02, 2008 at 12:37:18PM +0100, David Woodhouse wrote:
> On Fri, 2008-05-02 at 11:38 +1000, Neil Brown wrote:
> > Why is there a deadlock here?
> 
> Many file systems have their own locking, and lookup() can end up trying
> to re-take a lock which readdir() is already holding. In the JFFS2 case,
> it's the fs-internal inode mutex, which is required because the garbage
> collector can't use i_mutex for lock ordering reasons.
> 
> See also the readdir implementation and surrounding comments in
> fs/xfs/linux-2.6/xfs_file.c -- and the way GFS2 uses
> gfs2_glock_is_locked_by_me() to avoid the deadlock.
> 
> The annoying thing is that JFFS2 doesn't even _implement_ i_generation,
> so you get no more useful information out of the lookup() call anyway :)
> 
> > Both readdir and lookup are called with i_mutex held on the directory
> > so there should need to do any extra locking (he said, naively).  In
> > the readdirplus cases, i_mutex is held across both the readdir and the
> > lookup....
> > 
> > One problem with your proposed solution is that filehandles aren't all
> > the same length, so you cannot reliably leave space for them.
> 
> Not without moving stuff around during the postprocessing, I suppose.
> Which isn't very pretty -- but it's prettier than some of the hacks we
> have at the moment to avoid the deadlock.

This comes up periodically.  It would be great to finally get it fixed.
The last conversation I remember started at about:

	http://marc.info/?l=linux-kernel&m=119506226209056&w=2

Summary: one approach would be to define a ->readdirplus() that passes a
dentry to its equivalent of the ->filldir callback.  (Christoph points
out that returning a stat struct would be simpler.  But unfortunately we
need to check for mountpoints here, so that's not sufficient.)

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2008-05-02 14:08 UTC|newest]

Thread overview: 128+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-05-01 19:42 [RFC] Reinstate NFS exportability for JFFS2 David Woodhouse
2008-05-01 20:48 ` Christoph Hellwig
2008-05-01 22:44   ` David Woodhouse
2008-05-02  1:38     ` Neil Brown
2008-05-02  1:38       ` Neil Brown
2008-05-02 11:37       ` David Woodhouse
2008-05-02 11:37         ` David Woodhouse
2008-05-02 14:08         ` J. Bruce Fields [this message]
2008-05-02 14:08           ` J. Bruce Fields
2008-05-02 14:08           ` J. Bruce Fields
2008-07-31 21:54       ` David Woodhouse
2008-08-01  0:16         ` Neil Brown
2008-08-01  0:40           ` David Woodhouse
2008-08-01  0:40             ` David Woodhouse
2008-08-01  0:40             ` David Woodhouse
2008-08-01  0:52             ` David Woodhouse
2008-08-01  0:52               ` David Woodhouse
2008-08-01  0:52               ` David Woodhouse
2008-08-01  0:53             ` Chuck Lever
2008-08-01  0:53               ` Chuck Lever
2008-08-01  1:00               ` David Woodhouse
2008-08-01  1:00                 ` David Woodhouse
2008-08-01  1:00                 ` David Woodhouse
2008-08-01  1:31                 ` Chuck Lever
2008-08-01  1:31                   ` Chuck Lever
2008-08-01  1:31                   ` Chuck Lever
2008-08-01  8:13                   ` David Woodhouse
2008-08-01  8:13                     ` David Woodhouse
2008-08-01 13:35                   ` David Woodhouse
2008-08-01 13:35                     ` David Woodhouse
2008-08-01 13:56                     ` David Woodhouse
2008-08-01 13:56                       ` David Woodhouse
2008-08-01 13:56                       ` David Woodhouse
2008-08-01 16:05                       ` Chuck Lever
2008-08-01 16:05                         ` Chuck Lever
2008-08-01 16:19                         ` David Woodhouse
2008-08-01 16:19                           ` David Woodhouse
2008-08-01 17:47                           ` Chuck Lever
2008-08-01 17:47                             ` Chuck Lever
2008-08-02 18:26                             ` J. Bruce Fields
2008-08-02 18:26                               ` J. Bruce Fields
2008-08-02 20:42                               ` David Woodhouse
2008-08-02 20:42                                 ` David Woodhouse
2008-08-02 20:42                                 ` David Woodhouse
2008-08-02 21:33                                 ` J. Bruce Fields
2008-08-02 21:33                                   ` J. Bruce Fields
2008-08-03  8:39                                   ` David Woodhouse
2008-08-03  8:39                                     ` David Woodhouse
2008-08-03  8:39                                     ` David Woodhouse
2008-08-03 11:56                               ` Neil Brown
2008-08-03 11:56                                 ` Neil Brown
2008-08-03 11:56                                 ` Neil Brown
2008-08-03 17:15                                 ` Chuck Lever
2008-08-03 17:15                                   ` Chuck Lever
2008-08-04  1:03                                   ` Neil Brown
2008-08-04  1:03                                     ` Neil Brown
2008-08-04  6:19                                     ` Chuck Lever
2008-08-04  6:19                                       ` Chuck Lever
2008-08-04  6:19                                       ` Chuck Lever
2008-08-05  8:51                                       ` Dave Chinner
2008-08-05  8:51                                         ` Dave Chinner
2008-08-05  8:59                                         ` David Woodhouse
2008-08-05  8:59                                           ` David Woodhouse
2008-08-05  9:47                                           ` Dave Chinner
2008-08-05  9:47                                             ` Dave Chinner
2008-08-05 23:06                                         ` Neil Brown
2008-08-05 23:06                                           ` Neil Brown
2008-08-05 23:06                                           ` Neil Brown
2008-08-06  0:08                                           ` Dave Chinner
2008-08-06  0:08                                             ` Dave Chinner
2008-08-06  0:08                                             ` Dave Chinner
2008-08-06 19:56                                             ` J. Bruce Fields
2008-08-06 19:56                                               ` J. Bruce Fields
2008-08-06 20:10                                               ` David Woodhouse
2008-08-06 20:10                                                 ` David Woodhouse
2008-08-09 16:47                                                 ` David Woodhouse
2008-08-09 16:47                                                   ` David Woodhouse
2008-08-09 16:47                                                   ` David Woodhouse
2008-08-09 19:55                                                   ` David Woodhouse
2008-08-09 19:55                                                     ` David Woodhouse
2008-08-09 20:01                                                     ` [PATCH 1/4] Factor out nfsd_do_readdir() into its own function David Woodhouse
2008-08-09 20:01                                                       ` David Woodhouse
2008-08-09 20:01                                                       ` David Woodhouse
2008-08-09 20:07                                                       ` Christoph Hellwig
2008-08-09 20:07                                                         ` Christoph Hellwig
2008-08-09 20:07                                                         ` Christoph Hellwig
2008-08-09 20:02                                                     ` [PATCH 2/4] Copy XFS readdir hack into nfsd code David Woodhouse
2008-08-09 20:02                                                       ` David Woodhouse
2008-08-09 20:02                                                       ` David Woodhouse
2008-08-09 20:08                                                       ` Christoph Hellwig
2008-08-09 20:08                                                         ` Christoph Hellwig
2008-08-09 20:03                                                     ` [PATCH 3/4] Remove XFS buffered readdir hack David Woodhouse
2008-08-09 20:03                                                       ` David Woodhouse
2008-08-09 20:03                                                       ` David Woodhouse
2008-08-09 20:09                                                       ` Christoph Hellwig
2008-08-09 20:09                                                         ` Christoph Hellwig
2008-08-09 20:09                                                         ` Christoph Hellwig
2008-08-09 20:03                                                     ` [PATCH 4/4] Reinstate NFS exportability David Woodhouse
2008-08-09 20:03                                                       ` David Woodhouse
2008-08-09 20:03                                                       ` David Woodhouse
2008-08-09 20:10                                                       ` Christoph Hellwig
2008-08-09 20:10                                                         ` Christoph Hellwig
2008-08-09 20:10                                                         ` Christoph Hellwig
2008-08-06 19:56                                             ` [RFC] Reinstate NFS exportability for JFFS2 J. Bruce Fields
2008-08-04 18:41                                     ` J. Bruce Fields
2008-08-04 18:41                                       ` J. Bruce Fields
2008-08-04 22:37                                       ` Neil Brown
2008-08-04 22:37                                         ` Neil Brown
2008-08-17 18:22                                     ` Andreas Dilger
2008-08-17 18:22                                       ` Andreas Dilger
2008-08-17 18:22                                       ` Andreas Dilger
2008-08-01  2:14             ` Neil Brown
2008-08-01  2:14               ` Neil Brown
2008-08-01  8:50               ` David Woodhouse
2008-08-01  8:50                 ` David Woodhouse
2008-08-01  8:50                 ` David Woodhouse
2008-08-01 10:03               ` Al Viro
2008-08-01 10:03                 ` Al Viro
2008-08-01 10:03                 ` Al Viro
2008-08-01 23:11                 ` Neil Brown
2008-08-01 23:11                   ` Neil Brown
2008-08-01 23:11                   ` Neil Brown
2008-07-31 21:54       ` [PATCH 1/4] Factor out nfsd_do_readdir() into its own function David Woodhouse
2008-07-31 21:54       ` [PATCH 2/4] Copy XFS readdir hack into nfsd code, introduce FS_NO_LOOKUP_IN_READDIR flag David Woodhouse
2008-07-31 21:55       ` [PATCH 3/4] Switch XFS to using FS_NO_LOOKUP_IN_READDIR, remove local readdir hack David Woodhouse
2008-07-31 21:55       ` [PATCH 4/4] [JFFS2] Reinstate NFS exportability David Woodhouse
2008-07-31 21:55         ` David Woodhouse
2008-07-31 21:55         ` David Woodhouse

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080502140804.GA16959@fieldses.org \
    --to=bfields@fieldses.org \
    --cc=dwmw2@infradead.org \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.