Fw: Re: Process hangs copying large file to cifs filesystem

Fw: Re: Process hangs copying large file to cifs filesystem

[Andrew Morton]

This person's CIFS hang is happening because the hlist_unhashed() test in
__mark_inode_dirty() is returning true, so the inode never gets itself onto
sb->s_dirty.  The writeback paths have no way to write out the pagecache.

a) Why are CIFS inodes not hashed?

b) Why does __mark_inode_dirty() skip unhashed inodes anyway?


Taking the hlist_unhashed() test out fixed it up, although CIFS only wants
to write stuff out at 2 Mbytes/sec on 100bT, whereas NFS does wirespeed...



Begin forwarded message:

Date: Wed, 23 Jun 2004 12:44:54 +0100
From: Nuno Ferreira <nuno.ferreira@graycell.biz>
To: Jens Axboe <axboe@suse.de>
Cc: Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: Process hangs copying large file to cifs filesystem


On Sex, 2004-05-28 at 17:04 +0100, Nuno Ferreira wrote:
> On Sex, 2004-05-28 at 16:22 +0200, Jens Axboe wrote:
> > On Fri, May 28 2004, Nuno Ferreira wrote:
> > > On Qui, 2004-05-27 at 16:45 +0100, Nuno Ferreira wrote:
> > > > Hi,
> > > > I'm trying to copy a large file (200Mb or bigger) from an ext3
> > > > filesystem to a windows share mounted using CIFS and the cp process
> > > > hangs, sometimes for a long time (several minutes).
> > > > Calling ps, I can see that it's blocking on blk_congestion_wait. 
> [...]
> > 
> > A sysrq-t back trace of that process would be interesting to see.

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[viro]

On Fri, Jun 25, 2004 at 01:29:31AM -0700, Andrew Morton wrote:
> 
> This person's CIFS hang is happening because the hlist_unhashed() test in
> __mark_inode_dirty() is returning true, so the inode never gets itself onto
> sb->s_dirty.  The writeback paths have no way to write out the pagecache.
> 
> a) Why are CIFS inodes not hashed?

Presumably because it never does hash lookups.  I wonder where it gets
inumbers from, though - keeping them unique would at least require being
able to see what's already in-core.

> b) Why does __mark_inode_dirty() skip unhashed inodes anyway?

Ask Linus.  I've always treated that as a behaviour filesystems might be
using, and IIRC in some cases that was deliberately used.  I have my
suspicions about the origin of that animal, but that was before my time
(during 2.1.50ish rewrite of inode.c) and inode.c is one place I try
to touch as rarely as possible...

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Andrew Morton]

viro@parcelfarce.linux.theplanet.co.uk wrote:
>
> On Fri, Jun 25, 2004 at 01:29:31AM -0700, Andrew Morton wrote:
> > 
> > This person's CIFS hang is happening because the hlist_unhashed() test in
> > __mark_inode_dirty() is returning true, so the inode never gets itself onto
> > sb->s_dirty.  The writeback paths have no way to write out the pagecache.
> > 
> > a) Why are CIFS inodes not hashed?
> 
> Presumably because it never does hash lookups.  I wonder where it gets
> inumbers from, though - keeping them unique would at least require being
> able to see what's already in-core.

hm.  Well cifs inodes seem to have unique i_ino's somehow.

> > b) Why does __mark_inode_dirty() skip unhashed inodes anyway?
> 
> Ask Linus.  I've always treated that as a behaviour filesystems might be
> using, and IIRC in some cases that was deliberately used.  I have my
> suspicions about the origin of that animal, but that was before my time
> (during 2.1.50ish rewrite of inode.c) and inode.c is one place I try
> to touch as rarely as possible...

Linus, why does __mark_inode_dirty() skip unhashed inodes?

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Linus Torvalds]

On Fri, 25 Jun 2004, Andrew Morton wrote:
> 
> Linus, why does __mark_inode_dirty() skip unhashed inodes?

Hah. I wouldn't remember for the life of me. There could be multiple 
reasons, all of which I've forgotten:

 - ramfs deadlocks on "sync()", since it can't write out inodes

 - stale and bad inodes get unhashed, and end up being unwritable.

 - totally stale reason that doesn't exist any more.

I suspect #3 migth well be true, but it's a scary change to try.

Anybody else remember better than me?

		Linus

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Fri, 25 Jun 2004, Andrew Morton wrote:
> > 
> > Linus, why does __mark_inode_dirty() skip unhashed inodes?
> 
> Hah. I wouldn't remember for the life of me. There could be multiple 
> reasons, all of which I've forgotten:
> 
>  - ramfs deadlocks on "sync()", since it can't write out inodes

hm.  ramfs oopses on null ->writepage without the check.  I'll fix that up.

>  - stale and bad inodes get unhashed, and end up being unwritable.

Maybe we should check inode sanity in the writing path, rather than the
dirtying path.  We already check is_bad_inode() in write_inode().

>  - totally stale reason that doesn't exist any more.
> 
> I suspect #3 migth well be true, but it's a scary change to try.

It is indeed scary, yes.  I think the path of least resistance here is for
cifs to start hashing dem inodes.

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Linus Torvalds]

On Fri, 25 Jun 2004, Linus Torvalds wrote:
> 
>  - ramfs deadlocks on "sync()", since it can't write out inodes
> 
>  - stale and bad inodes get unhashed, and end up being unwritable.
> 
>  - totally stale reason that doesn't exist any more.

There might also be a race on "get rid of inode" from the filesystem
standpoint, and unhashing the inode would be the atomic "now nobody will
call us any more" point.  NFS exports etc in particular tend to have all
these interesting things where kNFSd ends up touching inodes even after
they have been unliked, because it uses the magic "look up by inode"  
interace.

Although I think the I_CLEARING bits etc should also act as atomic points 
of "nobody will touch this any more", but what do I know?

Anyway, I do believe that if we remove the check for hashedness, we'd need 
to really stress it over a long period in 2.7-like circumstances before we 
do it in a "real" kernel.

		Linus

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Fri, 25 Jun 2004, Linus Torvalds wrote:
> > 
> >  - ramfs deadlocks on "sync()", since it can't write out inodes
> > 
> >  - stale and bad inodes get unhashed, and end up being unwritable.
> > 
> >  - totally stale reason that doesn't exist any more.
> 
> There might also be a race on "get rid of inode" from the filesystem
> standpoint, and unhashing the inode would be the atomic "now nobody will
> call us any more" point.  NFS exports etc in particular tend to have all
> these interesting things where kNFSd ends up touching inodes even after
> they have been unliked, because it uses the magic "look up by inode"  
> interace.

hm, but we've called ->dirty_inode() on it prior to testing the unhashedness.

> Although I think the I_CLEARING bits etc should also act as atomic points 
> of "nobody will touch this any more", but what do I know?

That should be so.

> Anyway, I do believe that if we remove the check for hashedness, we'd need 
> to really stress it over a long period in 2.7-like circumstances before we 
> do it in a "real" kernel.
> 

Yes, this will explode in our faces for sure.  I was going to stick a
printk in there as a starting point, but ramfs might be a problem.  It
would, I think, be cleaner to permit ramfs inodes to be dirtied, but refuse
to write them out due to memory-backedness.  I'll play with it a bit.

Re: Fw: Re: Process hangs copying large file to cifs filesystem

[Andrew Morton]

Steven French <sfrench@us.ibm.com> wrote:
>
> > It is indeed scary, yes.  I think the path of least resistance here is 
> for
> > cifs to start hashing dem inodes.
> 
> Yes, agreed.  I will test that minor change, to call insert_inode_hash to 
> new cifs inodes, this weekend.
> 

Great, thanks.   Other things:

- cifs_commit_write() should be running set_page_dirty() in the
  !PageUptodate() case as well, yes?

- Currently, a cifs set_page_dirty() falls into
  __set_page_dirty_buffers().  That's OK, but incurs more locking than is
  needed.

  Suggest you make cifs_addr_ops.set_page_dirty point at
  __set_page_dirty_nobuffers().