Re: [PATCH V12 05/12] famfs: Introduce file_operations read/write

On 26/08/05 10:14PM, Darrick J. Wong wrote:
> On Mon, Aug 03, 2026 at 02:29:07AM +0000, John Groves wrote:
> > From: John Groves <[email protected]>
> > 
> > This commit introduces fs/famfs/famfs_file.c and the famfs
> > file_operations for read/write.
> > 
> > This is not usable yet because:
> > 
> > * It calls dax_iomap_rw() with NULL iomap_ops (which will be
> >   introduced in a subsequent commit).
> > * famfs_ioctl() is coming in a later commit, and it is necessary
> >   to map a file to a memory allocation.
> > 
> > Signed-off-by: John Groves <[email protected]>
> > ---
> >  fs/famfs/Makefile         |   2 +-
> >  fs/famfs/famfs_file.c     | 138 ++++++++++++++++++++++++++++++++++++++
> >  fs/famfs/famfs_inode.c    |   2 +-
> >  fs/famfs/famfs_internal.h |   2 +
> >  4 files changed, 142 insertions(+), 2 deletions(-)
> >  create mode 100644 fs/famfs/famfs_file.c
> > 
> > diff --git a/fs/famfs/Makefile b/fs/famfs/Makefile
> > index 62230bcd6793..8cac90c090a4 100644
> > --- a/fs/famfs/Makefile
> > +++ b/fs/famfs/Makefile
> > @@ -2,4 +2,4 @@
> >  
> >  obj-$(CONFIG_FAMFS) += famfs.o
> >  
> > -famfs-y := famfs_inode.o
> > +famfs-y := famfs_inode.o famfs_file.o
> > diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> > new file mode 100644
> > index 000000000000..e192b573c51f
> > --- /dev/null
> > +++ b/fs/famfs/famfs_file.c
> > @@ -0,0 +1,138 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * famfs - dax file system for shared fabric-attached memory
> > + *
> > + * Copyright 2023-2024 Micron Technology, Inc.
> > + *
> > + * This file system, originally based on ramfs the dax support from xfs,
> > + * is intended to allow multiple host systems to mount a common file system
> > + * view of dax files that map to shared memory.
> > + */
> > +
> > +#include <linux/fs.h>
> > +#include <linux/mm.h>
> > +#include <linux/dax.h>
> > +#include <linux/iomap.h>
> > +
> > +#include "famfs_internal.h"
> > +
> > +/*********************************************************************
> > + * file_operations
> > + */
> > +
> > +/* Reject I/O to files that aren't in a valid state */
> > +static ssize_t
> > +famfs_file_invalid(struct inode *inode)
> > +{
> > +	if (!IS_DAX(inode)) {
> > +		pr_debug("%s: inode %llx IS_DAX is false\n",
> > +			 __func__, (u64)inode);
> > +		return -ENXIO;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static ssize_t
> > +famfs_rw_prep(struct kiocb *iocb, struct iov_iter *ubuf)
> > +{
> > +	struct inode *inode = iocb->ki_filp->f_mapping->host;
> > +	struct super_block *sb = inode->i_sb;
> > +	struct famfs_fs_info *fsi = sb->s_fs_info;
> > +	size_t i_size = i_size_read(inode);
> > +	size_t count = iov_iter_count(ubuf);
> > +	size_t max_count;
> > +	ssize_t rc;
> > +
> > +	if (fsi->deverror)
> > +		return -ENODEV;
> > +
> > +	rc = famfs_file_invalid(inode);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* Avoid unsigned underflow if position is past EOF */
> > +	if (iocb->ki_pos >= i_size)
> > +		max_count = 0;
> > +	else
> > +		max_count = i_size - iocb->ki_pos;
> > +
> > +	if (count > max_count)
> > +		iov_iter_truncate(ubuf, max_count);
> > +
> > +	if (!iov_iter_count(ubuf))
> > +		return 0;
> > +
> > +	return rc;
> > +}
> > +
> > +static ssize_t
> > +famfs_dax_read_iter(struct kiocb *iocb, struct iov_iter	*to)
> > +{
> > +	struct inode *inode = iocb->ki_filp->f_mapping->host;
> > +	ssize_t rc;
> > +
> > +	/* dax_iomap_rw() requires i_rwsem held (shared for read) */
> > +	inode_lock_shared(inode);
> > +	rc = famfs_rw_prep(iocb, to);
> > +	if (rc || !iov_iter_count(to)) {
> > +		inode_unlock_shared(inode);
> > +		return rc;
> > +	}
> > +
> > +	rc = dax_iomap_rw(iocb, to, NULL /*&famfs_iomap_ops */);
> > +	inode_unlock_shared(inode);
> > +
> > +	file_accessed(iocb->ki_filp);
> 
> Is it really accessed if rc != 0?

Good point; looks like only if rc > 0. Will update, thanks.

> 
> > +	return rc;
> > +}
> > +
> > +/**
> > + * famfs_dax_write_iter()
> > + *
> > + * We need our own write-iter in order to prevent append
> > + *
> > + * @iocb:
> > + * @from: iterator describing the user memory source for the write
> > + */
> > +static ssize_t
> > +famfs_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > +{
> > +	struct inode *inode = iocb->ki_filp->f_mapping->host;
> > +	struct famfs_fs_info *fsi = inode->i_sb->s_fs_info;
> > +	ssize_t rc;
> > +
> > +	if (!famfs_opt_enabled(fsi, FAMFS_OPT_WRITE))
> > +		return -EPERM;
> > +
> > +	/* dax_iomap_rw() requires i_rwsem held (exclusive for write) */
> > +	inode_lock(inode);
> > +	rc = famfs_rw_prep(iocb, from);
> > +	if (rc || !iov_iter_count(from)) {
> > +		inode_unlock(inode);
> > +		return rc;
> > +	}
> > +
> > +	rc = dax_iomap_rw(iocb, from, NULL /*&famfs_iomap_ops*/);
> 
> What happens if you pass a null iomap ops?  TBH I was expecting you to
> define the iomap ops with a dummy ->iomap_begin that returns EIO or
> something.

If we actually called dax_iomap_rw() with null iomap ops, it would
hork. However, if we called it with null iomap_ops->iomap_begin it
will also hork. I could introduce the dax_iomap_rw call later, when
sufficient code is in, but that might require (void) declarations
to squelch the compiler about unreferenced stuff.

So dummy iomap_ops won't actually work. My objective was to drop in
bite-sized chunks that were functionally coherent, until it's complete
(in commit 12).

I suspect multiple of these commits would do something bad if you
tried to run them before you had all.

Given all that, I'm inclined to leave it alone. 

Hmm, I could keep things where all commits compile, but cause 
module_init to fail until all commits are in. Then it couldn't do 
any harm to try to try running incomplete famfs.

I think I'll do that...

> 
> > +	inode_unlock(inode);
> > +	return rc;
> 
> Do you need to update mtime here?
> 
> --D

Yeah, I guess I should - will do.

FYI file times in famfs have limited usefulness, because they don't
propagate in the cluster.

<snip>