On Mon, Mar 25, 2019 at 12:34:59PM -0700, Chenbo Feng wrote: [snip]
Also what is the benefit of having st_blocks from stat? AFAIK, that is the same as the buffer's size which does not change for the lifetime of the buffer. In your patch you're doing this when 'struct file' is created which AIUI is what reflects in the st_blocks:
- inode_set_bytes(inode, dmabuf->size);
Can some of the use cases / data be trimmed down? I think so. For example, I never understood what we do with map_files here (or why). It is perfectly fine to just get the data from /proc/<pid>/fd and /proc/<pid>/maps. I guess the map_files bit is for consistency?
It just occured to me that since /proc/<pid/maps provides an inode number as one of the fields, so indeed an inode per buf is a very good idea for the user to distinguish buffers just by reading /proc/<pid/<maps> alone..
I also, similar to you, don't think map_files is useful for this usecase. map_files are just symlinks named as memory ranges and pointing to a file. In this case the symlink will point to default name "dmabuf" that doesn't exist. If one does stat(2) on a map_file link, then it just returns the inode number of the symlink, not what the map_file is pointing to, which seems kind of useless.
I might be wrong but I don't think we did anything special for the map_files in this patch. I think the confusion might be from commit message where Greg mentioned the map_files to describe the behavior of shmem buffer when comparing it with dma-buf. The file system implementation and the file allocation action in this patch are just some minimal effort to associate each dma_buf object with an inode and properly populate the size information in the file object. And we didn't use proc/pid/map_files at all in the android implementation indeed.
You are right.
I am not against adding of inode per buffer, but I think we should have this debate and make the right design choice here for what we really need.
sure.
Right, so just to summarize:
- The intention here is /proc/<pid>/maps will give uniqueness (via the inode number) between different memory ranges. That I think is the main benefit of the patch.
- stat gives the size of buffer as does fdinfo
- fdinfo is useful to get the reference count of number of sharers of the buffer.
- map_files is not that useful for this usecase but can be made useful if we can name the underlying file's dentry to something other than "dmabuf".
- GET_NAME is not needed since fdinfo already has the SET_NAMEd name.
Do you agree?
Thanks for summarize it, I will look into the GET_NAME/SET_NAME ioctl to make it more useful as you suggested above. Also, I will try to add some test to verify the behavior.
Sounds great, thanks!
- Joel