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

[NeilBrown]

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..

https://www.spinics.net/lists/linux-nfs/msg62603.html

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
    sync

 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?

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature