Re: [PATCH 1/2] loop: use filp_close() rather than fput()

From: NeilBrown
Date: Sun Jun 18 2017 - 00:30:43 EST

On Sat, Jun 17 2017, Al Viro wrote:

> On Fri, Jun 16, 2017 at 03:02:09PM +1000, NeilBrown wrote:
>> When a loop device is being shutdown the backing file is
>> closed with fput(). This is different from how close(2)
>> closes files - it uses filp_close().
>> The difference is important for filesystems which provide a ->flush
>> file operation such as NFS. NFS assumes a flush will always
>> be called on last close, and gets confused otherwise.
> Huh? You do realize that mmap() + close() + modify + msync() + munmap()
> will have IO done *after* the last flush, right?

Yes I do ... or rather I did. I didn't make that connection this time.

The sequence you describe causes exactly the same sort of problem.
I sent a patch to Trond to add a vfs_fsync() call to nfs_file_release()
but he claims the current behaviour is "working as expected". I didn't
quite know what to make of that..

To provide the full picture:
When an NFS file has dirty pages, they (indirectly) hold extra
references on the superblock, using nfs_sb_active().
This means that when the filesystem is unmounted, the superblock
remains active until all the writes complete. This contrasts with
every other filesystems where all writes will complete before the
umount returns.

When you open/write/close, there will be no dirty pages at umount time
(because close() flushes) so this doesn't cause a problem. But when
you mmap, or use a loop device, then dirty pages can still be around to
keep the superblock alive.

The observable symptom that brought this to my attention was that
umount -a -t nfs
disable network

can hang in sync, because the NFS filesystems can still be waiting to
write out data.

If nfs_file_release() adds vfs_fsync(), or maybe if __fput() calls
filp->f_op->flush(), then loop.c wouldn't need to use filp_close().

Which would you prefer?


Attachment: signature.asc
Description: PGP signature